Skip to content

make test/integration:http_subset_lb_integration_test IP version envi…#8288

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
stevenzzzz:fix-failing-test
Sep 19, 2019
Merged

make test/integration:http_subset_lb_integration_test IP version envi…#8288
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
stevenzzzz:fix-failing-test

Conversation

@stevenzzzz
Copy link
Copy Markdown
Contributor

…ronment aware.

Signed-off-by: Xin Zhuang stevenzzz@google.com

Description: fix issue #8284
Risk Level: LOW
Testing: unit test
Docs Changes: N/A
Release Notes: N/A
Fixes 8284

…ronment aware.

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
Signed-off-by: Xin Zhuang <stevenzzz@google.com>
@stevenzzzz
Copy link
Copy Markdown
Contributor Author

/assign zuercher

@zuercher
Copy link
Copy Markdown
Member

Nothing about this test depends on which IP version is used (except that it fails if IPv4 isn't available). Parameterizing it will just make it take twice as long without any benefit.

What do you think of just replacing Network::Address::IpVersion::v4 with a call to a static helper function that returns the first value from getIpVersionsForTest()or just getIpVersionsForTest().front()? You can use the version_ field (from BaseIntegrationTest) to set the address in the config.

@stevenzzzz
Copy link
Copy Markdown
Contributor Author

Nothing about this test depends on which IP version is used (except that it fails if IPv4 isn't available). Parameterizing it will just make it take twice as long without any benefit.
I think the use of TestEnvironment::getIpVersionsForTest() will make sure it's tested with available IP versions. I'd actually prefer it has more coverage over different IP versions in case the code behaves differently when backends connection Ip version changes, right now the test finishes in 5s on my box.

What do you think of just replacing Network::Address::IpVersion::v4 with a call to a static helper function that returns the first value from getIpVersionsForTest()or just getIpVersionsForTest().front()? You can use the version_ field (from BaseIntegrationTest) to set the address in the config.
This should work, but WDYT we keep it this way.

@stevenzzzz
Copy link
Copy Markdown
Contributor Author

/assign htuch

@htuch
Copy link
Copy Markdown
Member

htuch commented Sep 18, 2019

+1 to @zuercher suggestion.

…egration_test

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
@stevenzzzz
Copy link
Copy Markdown
Contributor Author

alright, PTAL.
Much smaller change now.

Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit 0e63f5a into envoyproxy:master Sep 19, 2019
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Sep 24, 2019
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019
@stevenzzzz stevenzzzz deleted the fix-failing-test branch October 31, 2019 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants