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
test: not validating all protos in every test #24172
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
/retest |
Retrying Azure Pipelines: |
/retest |
Retrying Azure Pipelines: |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, overall I think this change is ok, just highlighted 2 questions regarding one of the test changes and I'd like to hear your thoughts.
@@ -47,6 +48,7 @@ BaseIntegrationTest::BaseIntegrationTest(const InstanceConstSharedPtrFn& upstrea | |||
version_(version), upstream_address_fn_(upstream_address_fn), | |||
config_helper_(version, *api_, config), | |||
default_log_level_(TestEnvironment::getOptions().logLevel()) { | |||
Envoy::Server::validateProtoDescriptors(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to highlight 2 questions regarding this:
- Is calling this outside of the mutex ok?
- Could there be a use of one of the validated protos before the BaseIntergrationTest c'tor is executed?
I'm less worried about 2 because this change only impacts tests, and so I guess we will see failures.
For 1, I'm guessing this is only relevant to multi-threaded use-cases, and AFAIK threading only starts after the integration test is instantiated.
Feel free to correct me if I'm wrong on any of the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think we're largely checking things that are registered at or before start up, so this is fine
- yes, but then the test will fail with a slightly less helpful warning so if the goal is to fail (it's an ASSERT not RELEASE_ASSERT) we will achieve the goal just less helpfully :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending CI, thanks!
Currently proto_gen_validate pulls all protos into every unit test, and every unit test validates their existence on start up. This is pretty terrible. bufbuild/protoc-gen-validate#738 pulls this out for Envoy Mobile proto-gen-validate-less builds, and so breaks all the unit tests. Changing the Envoy test framework to only validate proto registration for integration tests.
Risk Level: low (should be test only)
Testing: #24151
Docs Changes: n/a
Release Notes: n/a