test: various test changes to support OS X builds by zuercher · Pull Request #1375 · envoyproxy/envoy

zuercher · 2017-08-02T04:31:30Z

test/common/network:

make fewer assumptions about event timing and sequence
use the offset of sun_path rather than assuming the size and number of preceding sockaddr fields
OS X treats ::/96 IPv6 addresses as IPv6-mapped IPv4 addresses (per a deprecated RFC); use a different address range so expected strings match across OS type

test/common/http: use a more recent date for log tests to avoid an OS X time_t limitation: struct tm before 1901-12-13 20:45:52 cannot be converted to time_t.

test/common/runtime: handle difference between cp on Linux and OS X

test/common/upstream: disable ring_hash_lb_test on OS X

test/exe: OS X reports abort signals slightly differently than Linux

test/test_common: remove getSomeLoopbackAddress (OS X does not, by default, support IPv4 loopback other than 127.0.0.1) since all tests bind port 0

(Split out from #1348, in support of #128.)

test/common/network: make fewer assumptions about event timing and sequence test/common/http: use a more recent date for log tests to avoid an OS X bug test/common/runtime: handle difference between cp on Linux and OS X test/common/upstream: disable ring_hash_lb_test on OS X test/exe: OS X reports abort signals slightly differently than Linux test/test_common: by default OS X does not support multiple IPv4 loopback addresses

mattklein123

Thanks for all these fixes. Few comments.

mattklein123 · 2017-08-02T18:13:53Z

  client_connection_->close(ConnectionCloseType::NoFlush);
-  dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
+
+  EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::RemoteClose))


Can we use disconnect(false) here?

mattklein123 · 2017-08-02T18:20:31Z

  StringUtil::strlcpy(sun.sun_path, "", sizeof sun.sun_path);
-  EXPECT_THROW(addressFromSockAddr(ss, sizeof(sa_family_t) + 1 + strlen(sun.sun_path)),
-               EnvoyException);
+  EXPECT_THROW(addressFromSockAddr(ss, offsetof(struct sockaddr_un, sun_path) + 1), EnvoyException);


To make this less magic can it be the same as rest of code which is offsetof(struct sockaddr_un, sun_path) + 1 + strlen(sun.sun_path) still?

mattklein123 · 2017-08-02T18:27:52Z

-ln -sf ${TEST_TMPDIR}/test/common/runtime/test_data/root/envoy/subdir ${TEST_TMPDIR}/test/common/runtime/test_data/root/envoy/badlink
+case `uname` in
+    Darwin)
+        mkdir -p ${TEST_TMPDIR}/${TEST_DATA}


Does this work on linux also? If so do we need the darwin switch here?

mattklein123 · 2017-08-02T18:30:07Z


 Address::InstanceConstSharedPtr getSomeLoopbackAddress(Address::IpVersion version) {
  if (version == Address::IpVersion::v4) {
+#ifdef __APPLE__


@jamessynge @htuch @hennna is there any point in really doing the random address thing for tests? Could we just delete that part and use canonical to remove ifdef?

One aspect of the rationale here was we would reduce the likelihood of port collision on the few tests that don't bind to port zero and hold the socket, but instead release immediately and expect the port not to be taken by someone else. It's not really helpful in IPv6 though and doesn't fix the underlying issue.

@jamessynge is probably the best person to comment on this.

AFAIK everything is now binding to zero? I would vote to just remove this and treat that case as a test bug if/when it happens.

Everything binds zero, but some test release temporarily after the bind (the bind zero is an allocation without hold). I agree with your suggestion.

htuch · 2017-08-02T21:41:59Z


  log->log(&request_headers_, &response_headers_, request_info_);
-  EXPECT_EQ("[1900-01-01T00:00:00.000Z] \"GET / HTTP/1.1\" 0 UF 1 2 3 - \"x.x.x.x\" "
+  EXPECT_EQ("[1999-01-01T00:00:00.000Z] \"GET / HTTP/1.1\" 0 UF 1 2 3 - \"x.x.x.x\" "


Can you including a reference to this bug's details in your commit message? Seems surprising OS X isn't able to work on dates in 1900. OTOH, there was no HTTP then :)

Sure. Turns out this isn't strictly a bug: the range of time_t is implementation defined.

htuch · 2017-08-02T21:44:47Z

-ln -sf ${TEST_TMPDIR}/test/common/runtime/test_data/root ${TEST_TMPDIR}/test/common/runtime/test_data/current
-ln -sf ${TEST_TMPDIR}/test/common/runtime/test_data/root/envoy/subdir ${TEST_TMPDIR}/test/common/runtime/test_data/root/envoy/badlink
+mkdir -p ${TEST_TMPDIR}/${TEST_DATA}
+cp -RfL ${TEST_DATA}/* ${TEST_TMPDIR}/${TEST_DATA}


While you're here, can you quote all variable dereferences for path WSP safety, e.g. `+mkdir -p "${TEST_TMPDIR}/${TEST_DATA}"?

htuch · 2017-08-02T21:48:06Z


 Address::InstanceConstSharedPtr getSomeLoopbackAddress(Address::IpVersion version) {
  if (version == Address::IpVersion::v4) {
+#ifdef __APPLE__


One aspect of the rationale here was we would reduce the likelihood of port collision on the few tests that don't bind to port zero and hold the socket, but instead release immediately and expect the port not to be taken by someone else. It's not really helpful in IPv6 though and doesn't fix the underlying issue.

@jamessynge is probably the best person to comment on this.

htuch · 2017-08-02T21:49:18Z

          EXPECT_GE(expected_chunk_size, data.length());
          filter_seen += data.length();
          data.drain(data.length());
+


Nit: Spurious blank line.

htuch · 2017-08-02T22:01:40Z

  }

-  void disconnect() {
+  void disconnect(bool async) {


This is really condition on whether we expect a remote close or not, rather than async?

mattklein123 · 2017-08-03T20:30:21Z

@zuercher this LGTM beyond just removing the ifdef/special case in the IP address allocation test code.

mattklein123

Nice, thanks.

Description: Sets up the GH workflow to execute python tests in CI. Also adds a logger as a parameter to `run_engine`. Risk Level: Low Testing: This! Signed-off-by: Cerek Hillen <chillen@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This commit adds indexing for referencegrants. The indexing key uses To Kind + namespace in referencegrant for indexing which would help fetch only related referencegrants given aiservicebackend and namespace. **Related Issues/PRs (if applicable)** Closes #1375 --------- Signed-off-by: siddharth1036 <siddharthshah1036@gmail.com>

zuercher added 3 commits August 1, 2017 21:16

Merge branch 'master' into osx-6-test-hardening

34d1b86

match type change from master

1555523

mattklein123 reviewed Aug 2, 2017

View reviewed changes

review comments

3e1b5ae

htuch reviewed Aug 2, 2017

View reviewed changes

review comments

bb4a3a3

remove getSomeLoopbackAddress

f36825f

mattklein123 approved these changes Aug 3, 2017

View reviewed changes

mattklein123 merged commit 5f46fd3 into envoyproxy:master Aug 3, 2017

zuercher deleted the osx-6-test-hardening branch August 10, 2017 16:06

Conversation

zuercher commented Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Aug 3, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zuercher commented Aug 2, 2017 •

edited

Loading