Add generic c++ integration test for uniform extension behavior. by oschaaf · Pull Request #533 · envoyproxy/nighthawk

oschaaf · 2020-09-14T08:41:46Z

Test basic behavioural properties of all test-server extensions. Any new extensions
may piggy-back on this by enlisting themselves. In a follow up we'll purge code
from the pre-existing tests that can now be deprecated.

Split out from #512

Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com

Prerequisite:

Land Add HttpFilterIntegrationTestBase #517

- Extracts shared needs between tested extensions. - Sanitized api, better naming. - Doc comments. - Support for testing POST request methods and entity bodies to check the alternative flow that will trigger in extensions when using that. Prelude to envoyproxy#512, which includes it and provides a means to see the end goal. Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Split out from envoyproxy#512 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> ------ Prerequisite: - [ ] Land envoyproxy#517

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…lude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…lude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

…r-prelude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Switch tests for these extensions to use the recent shared test facilities. Eliminate tests we generalized in envoyproxy#533. Split out from envoyproxy#512 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k · 2020-09-17T19:12:04Z

@wjuan-AFK please review and assign back to me once done.

…r-prelude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf · 2020-09-18T22:00:36Z

Can't reproduce the clang-tidy failure locally, and in CI it doesn't state a specific complaint.
/retest

repokitteh-read-only · 2020-09-18T22:00:40Z

🔨 rebuilding ci/circleci: clang_tidy (failed build)

🐱

Caused by: a #533 (comment) was created by @oschaaf.

see: more, trace.

oschaaf · 2020-09-18T22:18:42Z

Yup, clang-tidy flaked in CI for this PR, which is unusual :-( Filed #546 to put in on the radar.

wjuan-AFK · 2020-09-21T21:07:54Z

test/server/http_filter_base_test.cc

+// - Bad request-level json configuration.
+// We will be low on functional expectations for the extensions, but this will catch hangs and
+// ensure that bad configuration handling is in-place.
+TEST_P(HttpFilterBaseIntegrationTest, BasicExtensionFlows) {


Should we split these into separate tests? I'm not sure what the preference in the code base is, but it seems like each chunk is testing a different behaviour and might deserve a separate test?

I can see your point, and in general I agree that the clearer test names that arise out of more finely grained separate tests are better.
But in this case, when splitting this, up we would have to duplicate the parameter unpacking for the TEST_P params across the newly split up tests and/or add facilities to help with that.
This was also sort of pre-existing (as we can eliminate three different instances of similar code in the test code for three extensions).

So personally I lean towards the compact version as is is now. With the context above, are you comfortable with leaving this as-is?

Yeah, makes sense to me.

wjuan-AFK · 2020-09-21T21:10:00Z

test/server/http_filter_base_test.cc

+  EXPECT_EQ("200", response->headers().Status()->value().getStringView());
+  EXPECT_TRUE(response->body().empty());
+
+  const std::string kBadConfigErrorSentinel =


Why not use status instead of doing this substring check?

We do check the http response code, but then also look for this substring in the response body because we know it should look like that if the extension under test is using the shared configuration handling code. Does that sound good?

Yes. That also makes sense.

mum4k · 2020-09-22T20:17:46Z

test/server/http_filter_base_test.cc

+
+namespace Nighthawk {
+
+using namespace testing;


Do we need this, since most symbols from the namespace are qualified? E.g. ::testing::Combine.

If you do prefer to shorten them, can we list them explicitly instead of subscribing to the entire testing namespace which could cause a conflict and confuse someone in the future?

E.g. for symbols that repeat a lot we can:

using ::testing::Combine;
using ::testing::ValuesIn;

Done; added an anonymous namespace as well based on that it can only benefit us.

mum4k · 2020-09-22T20:24:50Z

test/server/http_filter_base_test.cc

+// - Bad request-level json configuration.
+// We will be low on functional expectations for the extensions, but this will catch hangs and
+// ensure that bad configuration handling is in-place.
+TEST_P(HttpFilterBaseIntegrationTest, BasicExtensionFlows) {


Adding to what @wjuan-AFK commented on - it would really help if we can separate these into individual test cases - one per behavior. Such approach simplifies future code changes, because it ensures that the tests won't be affected when a new behavior is added or that less tests are affected when a behavior is changed. Additionally when a behavior breaks, we get a clear signal which behavior broke, rather than a generic failure saying "the BasicExtensionFlows no longer works".

I see your point about duplicating the parameter unpacking. I wonder if we could place that into a separate helper function. If we figure out we can't, I think we should prefer more instances of smaller independent tests for individual behavior to shorter code that uses parametrised tests. In other words, if we need to drop the parametrised test approach and write few more test cases, I think that is a worthwhile price to pay for shorter and easier to maintain test cases.

Can we burn another cycle on this and see if we can find a win-win solution?

I will take a look. Hopefully a convenient helper will suffice to support this, and then I can split these out into different tests. I'd like to avoid the need to drop parameterized testing here if we can: we would not only have to duplicate the test(s) for our three current extensions, but also require that for future extensions that would want to piggy back on this.

Done in 5ff6d56.
@wjuan-AFK sorry, this was actually straight forward, and looking at the results, I rather like it better compared to what it was.

…r-prelude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

mum4k

Looks great, thank you for splitting them up.

Switch tests for these extensions to use the recent shared test facilities. Eliminate tests we generalize in #533. Split out from #512 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added 7 commits September 9, 2020 18:59

Merge remote-tracking branch 'upstream/master' into extension-refacto…

0c6f182

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

8d30820

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into extension-refacto…

4827909

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into extension-refacto…

5f18691

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add setRequestHeader

7eeb7e9

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Add generic c++ integration test for uniform extension behavior.

467e8a7

Split out from envoyproxy#512 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com> ------ Prerequisite: - [ ] Land envoyproxy#517

oschaaf mentioned this pull request Sep 14, 2020

Test-server extension: use shared configuration base and test facilities #512

Merged

9 tasks

oschaaf added the waiting-for-review A PR waiting for a review. label Sep 14, 2020

oschaaf marked this pull request as draft September 14, 2020 08:54

oschaaf removed the waiting-for-review A PR waiting for a review. label Sep 14, 2020

oschaaf added 4 commits September 14, 2020 23:36

Merge remote-tracking branch 'upstream/master' into extension-refacto…

e4a50bd

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

ca17f41

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge branch 'extension-refactor-prelude' into extension-refactor-pre…

9a2f69e

…lude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback sync

97322d8

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf force-pushed the extension-refactor-prelude-3 branch from 2900d37 to 97322d8 Compare September 15, 2020 12:16

oschaaf mentioned this pull request Sep 15, 2020

Add HttpFilterIntegrationTestBase #517

Merged

oschaaf added 5 commits September 16, 2020 22:29

Merge remote-tracking branch 'upstream/master' into extension-refacto…

139cfff

…r-prelude Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

2aff84a

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge branch 'extension-refactor-prelude' into extension-refactor-pre…

fa2ff48

…lude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Sync file I forgot

f0c8eba

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Merge remote-tracking branch 'upstream/master' into extension-refacto…

3440fd9

…r-prelude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf added the waiting-for-review A PR waiting for a review. label Sep 17, 2020

oschaaf marked this pull request as ready for review September 17, 2020 06:51

oschaaf mentioned this pull request Sep 17, 2020

Use shared test facilities for time-tracking and dynamic-delay #541

Merged

1 task

mum4k requested a review from wjuan-AFK September 17, 2020 19:10

Merge remote-tracking branch 'upstream/master' into extension-refacto…

0f3110c

…r-prelude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf mentioned this pull request Sep 18, 2020

clang-tidy flake #546

Closed

wjuan-AFK reviewed Sep 21, 2020

View reviewed changes

wjuan-AFK previously approved these changes Sep 22, 2020

View reviewed changes

wjuan-AFK assigned mum4k Sep 22, 2020

mum4k requested changes Sep 22, 2020

View reviewed changes

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Sep 22, 2020

oschaaf added 2 commits September 22, 2020 23:09

Merge remote-tracking branch 'upstream/master' into extension-refacto…

ab3585b

…r-prelude-3 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

Review feedback

5ff6d56

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>

oschaaf dismissed wjuan-AFK’s stale review via 5ff6d56 September 22, 2020 21:44

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Sep 22, 2020

mum4k approved these changes Sep 23, 2020

View reviewed changes

mum4k merged commit 164d98b into envoyproxy:master Sep 23, 2020

Conversation

oschaaf commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mum4k commented Sep 17, 2020

Uh oh!

oschaaf commented Sep 18, 2020

Uh oh!

repokitteh-read-only bot commented Sep 18, 2020

Uh oh!

oschaaf commented Sep 18, 2020

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!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oschaaf commented Sep 14, 2020 •

edited

Loading