quic: fix Server's Preferred Address tests to when envoy.reloadable_features.quic_send_server_preferred_address_to_all_clients is false

[danzh2010]

Those tests were accidentally changed pass only if that runtime guard is true.

Risk Level: low
Testing: fixing tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

[danzh2010]

/assign @ggreenway

[ggreenway]

I don't understand; this flag isn't ever false in these tests, so it seems like this is dead code.

/wait-any

[danzh2010]

I don't understand; this flag isn't ever false in these tests, so it seems like this is dead code.

No, they are not dead code. Before this flag is introduced, these tests would make the client to send the connection option kSPAD to signal its support of SPA. This logic is accidentally removed when the flag is introduced, so these tests will fail when the flag is false.

[ggreenway]

so these tests will fail when the flag is false

But the flag is never false in these tests. Otherwise, they'd be failing in CI everytime.

[danzh2010]

so these tests will fail when the flag is false

But the flag is never false in these tests. Otherwise, they'd be failing in CI everytime.

The issue is that we flipped the flag internally and these tests were not happy.

[danzh2010]

so these tests will fail when the flag is false

But the flag is never false in these tests. Otherwise, they'd be failing in CI everytime.

The issue is that we flipped the flag internally and these tests were not happy.

I think all tests should be written in a way that is robust to random flag flips.

[danzh2010]

/retest

[ggreenway]

I think all tests should be written in a way that is robust to random flag flips.

Most/all of the envoy tests are written to assume the default value for the flags (or the test explicitly flips the flag to test the opposite case); I don't think anything like this has been done before.

[danzh2010]

I think all tests should be written in a way that is robust to random flag flips.

Most/all of the envoy tests are written to assume the default value for the flags (or the test explicitly flips the flag to test the opposite case); I don't think anything like this has been done before.

@alyssawilk who originally implemented runtime guard. If tests only work for default values, it will be hard time to turn off the flags when issues are reported with the flag.

[danzh2010]

/retest

[alyssawilk]

I support tests working with flipped flags. That said if the test doesn't work without server preferred address, why not just latch it true for these tests? then they'd pass even if the flag was default false.

[danzh2010]

We actually want to test the functionality with this flag set to false as well.

[alyssawilk]

if you want to test the functionality with it explicit false I encourage more tests like TEST_P(QuicHttpIntegrationTest, PreferredAddressRuntimeFlag) {
which explicitly set it to false.

You could have standard tests like that, and then have the test BUILD have
--runtime-feature-override-for-tests=quic_send_server_preferred_address_to_all_clients to handle internal import

[ggreenway]

I think if the goal is to make tests pass if the default flag value is changed, just override the flag value at the beginning of the test.

If the goal is to test the quiche client code in both modes, do the above, and add an additional copy of the test with the flag forced to the other mode and the required bits to make it work in that mode.

With how it's written now, there is code that is never run in envoy CI, so if someone makes changes there that don't work, we wouldn't know.