Add quic_test_output_impl.(h|cc) to QUICHE platform implementation. by wu-bin · Pull Request #6058 · envoyproxy/envoy

wu-bin · 2019-02-25T20:31:23Z

Description:

Add quic_test_output_impl.(h|cc) to QUICHE platform implementation.

If environment variable "QUIC_TEST_OUTPUT_DIR" is set, tests using the QuicRecordTestOutput() function can save test output files to the $QUIC_TEST_OUTPUT_DIR directory.

Risk Level: minimal: code not used yet
Testing:

[No test output:] bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all --define quiche=enabled --experimental_remap_main_repo

[Test output saved to /tmp:] bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all --define quiche=enabled --experimental_remap_main_repo --action_env=QUIC_TEST_OUTPUT_DIR=/tmp

Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]

Signed-off-by: Bin Wu <wub@google.com>

wu-bin · 2019-02-26T12:31:48Z

/assign @mpwarres

mpwarres

LGTM, modulo a nit and one question

source/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc

mpwarres · 2019-02-26T17:17:38Z

source/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc

+    output_dir += '/';
+  }
+
+  Envoy::Filesystem::InstanceImpl file_system;


It would be nice if the Filesystem::Instance could be obtained from Api::Api::fileSystem(), but I can't think of a way to do that, since we don't have access to the Api::Api instance here, and there's no good way to pass it down explicitly.

What you have here looks like best immediate solution to me, but leaving this comment in case Envoy folks have any other ideas. I do wonder if this will come up in other places as well though, e.g. if other platform impls need to make use of functionality abstracted under Api::Api. Maybe we need a singleton (ick) or other means to fetch the Api::Api when it can't be passed explicitly?

I'd love @mattklein123 take on this one.
I had a similar issue with something the server owned lately and I reaaaally wanted to make it a singleton too, but then again I love singletons about as much as Matt and Josh dislike them.

The code in this PR is purely for tests, IMHO, making test code more testable via a mock is going a little bit too far. Does it make any sense?

Fair enough, I'm fine with not using a mock and checking for the file directly.
I do think we will need a solution to the general Api::Api issue, but maybe we should deal with that separately from this PR? If we do that, we'll need to remember to circle back and update quic_test_output_impl.cc to not hardcode FilesystemImpl later on.

I think we are going to have to end up plumbing an Api into this platform abstraction somehow. Can the platform abstraction have shared state that is initialized? If we have that it should be quite easy to pass in an Api. cc @jmarantz

@mpwarres Agree we need a solution for the general Api issue, I think we can circle back when we have a concrete issue related to it. I still think quic_test_output_impl.cc can continue to write directly to real files. I found that test function TestEnvironment::writeStringToFileForTest is already doing something similar.

@mattklein123 The code in this PR is test only, all it does is to allow some unit tests to save some test output to somewhere(in this case, a file). We have a test of this test-code in quic_platform_test.cc(QuicPlatformTest.QuicTestOutput), I think the point of plumbing Api into here is to test this test-code better, which seems a little overkill?

@mattklein123 Most platform abstractions, including this one, do not have shared states. I won't be surprised if some of them end up needing a shared Api, I think we can deal with that when the need arises.

If this is only used in test code, can we put it in the test directory to make that clear? Can you have a test platform split out?

@mattklein123 I have tried moving it to the test directory previously, but gave up due to some complications. The issue is quic_test_output.h, which is part of upstream QUICHE, and built in cc_library "quic_platform_base" in bazel/external/quiche.BUILD, wants to include 'extensions/quic_listeners/quiche/platform/quic_test_output_impl.h', this path does not work if quic_test_output_impl.h is in the test directory.

Re: @mattklein123's earlier question, we could add shared state to the platform implementation that is initialized early on via an Init() call, which IIUC would amount to a singleton maintained within the QUICHE platform impl. If that seems reasonable, we can add that, though I think our preference would be to do it separate from this PR, if that's ok.

alyssawilk · 2019-02-27T15:19:07Z

source/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc

+    output_dir += '/';
+  }
+
+  Envoy::Filesystem::InstanceImpl file_system;


I'd love @mattklein123 take on this one.
I had a similar issue with something the server owned lately and I reaaaally wanted to make it a singleton too, but then again I love singletons about as much as Matt and Josh dislike them.

alyssawilk · 2019-02-27T15:20:24Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

+  quic::GetLogger().set_level(quic::INFO);
+
+  quic::QuicRecordTestOutput("quic_test_output.1", "output 1 content\n");
+  quic::QuicRecordTestOutput("quic_test_output.2", "output 2 content\n");


This just tests the fast-fail due to QUIC_TEST_OUTPUT_DIR not being set, right? It'd be nice to test that the filesystem work actually works and the various appends do as well.

Agreed. If we end up using Api::fileSystem(), the test could inject a mock Filesystem::Instance by way of that, which would simplify verifying this.

I tested it with --action_env=QUIC_TEST_OUTPUT_DIR=/tmp in the bazel command. (PR description has the full command).

Alternatively, we can do a setenv() at the beginning of the test, and check for the actual file content at the end of the test, which will make this test less manual. WDYT?

Yeah, I think having CI actually cover the code path would be good. I think setenv is probably the best bet but I'd be fine if there's a way to build it into the test definition too.

I've updated the test to setenv(), if the env is not already set, so the code is now covered by CI.
Checking the output is still manual though, because the test does not know the file path, which is generated by the QuicRecordTestOutput function internally.

Could we use the Envoy log utils (EXPECT_LOG_NOT_CONTAINS) to verify there was no warning logged, to make sure we didn't hit the fast fail paths?

I'd think we could verify existence of /tmp/quic_test_output.1 unless we expect that to change under the hood

Could we use the Envoy log utils (EXPECT_LOG_NOT_CONTAINS) to verify there was no warning logged, to make sure we didn't hit the fast fail paths?

Good idea, done.

I'd think we could verify existence of /tmp/quic_test_output.1 unless we expect that to change under the hood

The filename is not visible to the test, but I added EXPECT_LOG_CONTAINS, which make sure some test output file is generated.

source/extensions/quic_listeners/quiche/platform/BUILD

…impl Signed-off-by: Bin Wu <wub@google.com>

Signed-off-by: Bin Wu <wub@google.com>

defined in the program environment, set it to "/tmp" such that test output file is always created. Signed-off-by: Bin Wu <wub@google.com>

Signed-off-by: Bin Wu <wub@google.com>

…impl Signed-off-by: Bin Wu <wub@google.com>

mpwarres

LGTM

mpwarres · 2019-03-05T18:21:41Z

source/extensions/quic_listeners/quiche/platform/quic_test_output_impl.cc

+    output_dir += '/';
+  }
+
+  Envoy::Filesystem::InstanceImpl file_system;


Re: @mattklein123's earlier question, we could add shared state to the platform implementation that is initialized early on via an Init() call, which IIUC would amount to a singleton maintained within the QUICHE platform impl. If that seems reasonable, we can add that, though I think our preference would be to do it separate from this PR, if that's ok.

alyssawilk

Looks good - just one final test request

alyssawilk · 2019-03-05T21:51:55Z

test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc

+  quic::GetLogger().set_level(quic::INFO);
+
+  quic::QuicRecordTestOutput("quic_test_output.1", "output 1 content\n");
+  quic::QuicRecordTestOutput("quic_test_output.2", "output 2 content\n");


Could we use the Envoy log utils (EXPECT_LOG_NOT_CONTAINS) to verify there was no warning logged, to make sure we didn't hit the fast fail paths?

I'd think we could verify existence of /tmp/quic_test_output.1 unless we expect that to change under the hood

…impl Signed-off-by: Bin Wu <wub@google.com>

Signed-off-by: Bin Wu <wub@google.com>

…impl Signed-off-by: Bin Wu <wub@google.com>

wu-bin · 2019-03-07T13:58:44Z

Test has been updated to make sure some output file is generated. PTAL. @mpwarres @alyssawilk

mpwarres · 2019-03-07T15:06:45Z

Test has been updated to make sure some output file is generated. PTAL. @mpwarres @alyssawilk

LGTM, thanks!

Add quic_test_output_impl.(h|cc) to QUICHE platform implementation.

52b7311

Signed-off-by: Bin Wu <wub@google.com>

wu-bin requested review from alyssawilk and mattklein123 as code owners February 25, 2019 20:31

repokitteh-read-only bot assigned mpwarres Feb 26, 2019

mpwarres reviewed Feb 26, 2019

View reviewed changes

alyssawilk reviewed Feb 27, 2019

View reviewed changes

source/extensions/quic_listeners/quiche/platform/BUILD Show resolved Hide resolved

wu-bin added 6 commits February 27, 2019 11:04

Merge remote-tracking branch 'upstream/master' into quic_test_output_…

ec02e27

…impl Signed-off-by: Bin Wu <wub@google.com>

Update per comments.

fe2af18

Signed-off-by: Bin Wu <wub@google.com>

In test QuicPlatformTest.QuicTestOutput, if QUIC_TEST_OUTPUT_DIR is not

1a2711b

defined in the program environment, set it to "/tmp" such that test output file is always created. Signed-off-by: Bin Wu <wub@google.com>

Remove a unnecessary getenv in test QuicPlatformTest.QuicTestOutput.

80837ae

Signed-off-by: Bin Wu <wub@google.com>

Merge remote-tracking branch 'upstream/master' into quic_test_output_…

3cf7f7d

…impl Signed-off-by: Bin Wu <wub@google.com>

Merge remote-tracking branch 'upstream/master' into quic_test_output_…

c65b728

…impl Signed-off-by: Bin Wu <wub@google.com>

mpwarres previously approved these changes Mar 5, 2019

View reviewed changes

alyssawilk reviewed Mar 5, 2019

View reviewed changes

wu-bin added 2 commits March 6, 2019 16:16

Merge remote-tracking branch 'upstream/master' into quic_test_output_…

c677256

…impl Signed-off-by: Bin Wu <wub@google.com>

Add EXPECT_LOG(_NOT)_CONTAINS to QuicPlatformTest.QuicTestOutput.

e268e03

Signed-off-by: Bin Wu <wub@google.com>

wu-bin dismissed mpwarres’s stale review via e268e03 March 6, 2019 21:30

Merge remote-tracking branch 'upstream/master' into quic_test_output_…

f0d1018

…impl Signed-off-by: Bin Wu <wub@google.com>

alyssawilk approved these changes Mar 7, 2019

View reviewed changes

alyssawilk merged commit 088d978 into envoyproxy:master Mar 7, 2019

mpwarres mentioned this pull request Apr 19, 2019

thread: remove ThreadFactorySingleton #6658

Merged

Conversation

wu-bin commented Feb 25, 2019

Uh oh!

wu-bin commented Feb 26, 2019

Uh oh!

mpwarres left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-bin Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpwarres left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-bin commented Mar 7, 2019

Uh oh!

mpwarres commented Mar 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wu-bin Mar 6, 2019 •

edited

Loading