Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Sep 19, 2019

Conversation

stevenzzzz
Copy link
Contributor

…ronment aware.

Signed-off-by: Xin Zhuang [email protected]

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

@stevenzzzz
Copy link
Contributor Author

/assign zuercher

@zuercher
Copy link
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
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
Contributor Author

/assign htuch

@htuch
Copy link
Member

htuch commented Sep 18, 2019

+1 to @zuercher suggestion.

@stevenzzzz
Copy link
Contributor Author

alright, PTAL.
Much smaller change now.

Copy link
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