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.

Sign up for GitHub

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

Jump to bottom

test: not validating all protos in every test #24172

Merged
merged 2 commits into from
Nov 23, 2022
Merged

test: not validating all protos in every test #24172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
361480d
test: not validating all protos in every test
alyssawilk Nov 23, 2022
f65b50f
Merge branch 'main' into proto_fixup
alyssawilk Nov 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions source/exe/process_wide.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ struct InitData {
InitData& processWideInitData() { MUTABLE_CONSTRUCT_ON_FIRST_USE(InitData); };
} // namespace

ProcessWide::ProcessWide() {
ProcessWide::ProcessWide(bool validate_proto_descriptors) {
// Note that the following lock has the dual use of making sure that initialization is complete
// before a second caller can enter and leave this function.
auto& init_data = processWideInitData();
Expand All @@ -31,7 +31,9 @@ ProcessWide::ProcessWide() {
// TODO(mattklein123): Audit the following as not all of these have to be re-initialized in the
// edge case where something does init/destroy/init/destroy.
Event::Libevent::Global::initialize();
Envoy::Server::validateProtoDescriptors();
if (validate_proto_descriptors) {
Envoy::Server::validateProtoDescriptors();
}
Http::Http2::initializeNghttp2Logging();

// We do not initialize Google gRPC here -- we instead instantiate
Expand Down
2 changes: 1 addition & 1 deletion source/exe/process_wide.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace Envoy {
// e.g. c-ares. There should only ever be a single instance of this.
class ProcessWide {
public:
ProcessWide();
ProcessWide(bool validate_proto_descriptors = true);
~ProcessWide();
};

Expand Down
1 change: 1 addition & 0 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,6 +892,7 @@ envoy_cc_test_library(
"base_integration_test.h",
],
deps = [
"//source/server:proto_descriptors_lib",
"//source/extensions/request_id/uuid:config",
":autonomous_upstream_lib",
":fake_upstream_lib",
Expand Down
2 changes: 2 additions & 0 deletions test/integration/base_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "source/common/network/utility.h"
#include "source/extensions/transport_sockets/tls/context_config_impl.h"
#include "source/extensions/transport_sockets/tls/ssl_socket.h"
#include "source/server/proto_descriptors.h"

#include "test/integration/utility.h"
#include "test/test_common/environment.h"
Expand Down Expand Up @@ -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();
Copy link
Contributor

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:

  1. Is calling this outside of the mutex ok?
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I think we're largely checking things that are registered at or before start up, so this is fine
  2. 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 :-)

// This is a hack, but there are situations where we disconnect fake upstream connections and
// then we expect the server connection pool to get the disconnect before the next test starts.
// This does not always happen. This pause should allow the server to pick up the disconnect
Expand Down
2 changes: 1 addition & 1 deletion test/test_runner.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ int TestRunner::RunTests(int argc, char** argv) {
::testing::InitGoogleMock(&argc, argv);
// We hold on to process_wide to provide RAII cleanup of process-wide
// state.
ProcessWide process_wide;
ProcessWide process_wide(false);
// Add a test-listener so we can call a hook where we can do a quiescence
// check after each method. See
// https://github.com/google/googletest/blob/master/googletest/docs/advanced.md
Expand Down