Skip to content

tls: refactor tls tests#4958

Merged
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/tls_test_cases
Nov 5, 2018
Merged

tls: refactor tls tests#4958
mattklein123 merged 2 commits intoenvoyproxy:masterfrom
ramaraochavali:fix/tls_test_cases

Conversation

@ramaraochavali
Copy link
Copy Markdown
Contributor

@ramaraochavali ramaraochavali commented Nov 4, 2018

Description: One of the review suggestions in #4810 is to move some of the variable initialization to class level. Here is the related discussion . This PR addresses that feedback.
Risk Level: Low
Testing: Existing automated tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Rama <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@lizan @jmarantz fyi. Can you PTAL?

lizan
lizan previously approved these changes Nov 4, 2018
Copy link
Copy Markdown
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Thanks!

@lizan lizan requested a review from jmarantz November 4, 2018 09:11
Comment thread test/common/ssl/context_impl_test.cc Outdated
static void loadConfig(ServerContextConfigImpl& cfg, Event::TestTimeSystem& time_system) {
ContextManagerImpl manager(time_system);
Stats::IsolatedStoreImpl store;
static void loadConfig(ServerContextConfigImpl& cfg, ContextManagerImpl& manager,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not make this non-static so manager and store can be ref'd from class?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. Agree. We can change it like that.

Comment thread test/common/ssl/context_impl_test.cc Outdated
manager.createSslServerContext(store, cfg, std::vector<std::string>{}));
}

static void loadConfigV2(envoy::api::v2::auth::DownstreamTlsContext& cfg,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto...also time_system_

Comment thread test/common/ssl/context_impl_test.cc Outdated
}

static void loadConfigYaml(const std::string& yaml, Event::TestTimeSystem& time_system) {
static void loadConfigYaml(const std::string& yaml, ContextManagerImpl& manager,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here.

Signed-off-by: Rama <rama.rao@salesforce.com>
@ramaraochavali
Copy link
Copy Markdown
Contributor Author

@jmarantz made the methods non-static. PTAL.

Copy link
Copy Markdown
Contributor

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

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

nice, thanks!

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice!

@mattklein123 mattklein123 merged commit 989de30 into envoyproxy:master Nov 5, 2018
@ramaraochavali ramaraochavali deleted the fix/tls_test_cases branch November 5, 2018 03:41
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