[tests] Convert core e2e tests to gtest by ctiller · Pull Request #32603 · grpc/grpc

ctiller · 2023-03-11T22:50:01Z

Notes:

+trace fixtures haven't run since 2016, so they're disabled for now (7ad2d0b#diff-780fce7267c34170c1d0ea15cc9f65a7f4b79fefe955d185c44e8b3251cf9e38R76)
all current fixtures define FEATURE_MASK_SUPPORTS_AUTHORITY_HEADER and hence authority_not_supported has not been run in years - deleted
bad_hostname similarly hasn't been triggered in a long while, so deleted
load_reporting_hook has never been enabled, so deleted (https://github.com/grpc/grpc/blame/f23fb4cf3114787806c330985c8fb3213597a09b/test/core/end2end/generate_tests.bzl#L145-L148)
filter_latency & filter_status_code rely on global variables and so don't convert particularly cleanly - and their value seems marginal, so deleted

…zards

Automated fix for refs/heads/leaping-lizards

…g-lizards

Automated fix for refs/heads/leaping-lizards

…g-lizards

Automated fix for refs/heads/leaping-lizards

yijiem · 2023-04-03T21:24:41Z

+  CoreEnd2endTest::IncomingMessage client_message;
+  s.NewBatch(102)
+      .SendInitialMetadata({{"key3", "val3"}, {"key4", "val4"}})
+      .RecvMessage(client_message);


I think this is just me unfamiliar with the test or how gRPC works and probably unrelated to this change. Is there any difference between checking the payload of the client_message right after the next Step() on line 49 or checking it at the end?

Is it more correct to check it right after the next Step() which completes this batch?

I think we'd like to move these to just after the Step() function as soon as we can. It'd be better than what's here now. I avoided doing so because the earlier code didn't (mostly because it was easier to write the C89 at the time), and keeping that bit consistent made my first round of checking I'd gotten everything right a bit easier.

yijiem · 2023-04-03T21:34:11Z

+                 .Set(GRPC_ARG_INITIAL_RECONNECT_BACKOFF_MS, 1000)
+                 .Set(GRPC_ARG_MAX_RECONNECT_BACKOFF_MS, 1000)
+                 .Set(GRPC_ARG_MIN_RECONNECT_BACKOFF_MS, 5000));
+  SimpleDelayedRequestBody(*this);


Missed a comment // This timeout should be longer than a single retry; but aren't these 2 tests exactly the same? Should we remove one?

There's a benefit for clearer code!

yijiem · 2023-04-03T22:06:41Z

-              grpc_core::CqVerifier::Maybe{&seen_status});
-  cqv->Expect(grpc_core::CqVerifier::tag(104), true);
-  cqv->Verify();
+  test.Expect(1, CoreEnd2endTest::Maybe{&seen_status});


This should be CqVerifier::Maybe{&seen_status} no?

CoreEnd2endTest aliases all of these, so we should use the alias. I expect to remove those aliases 'sometime' - but not sure how that plays out just yet.

yijiem · 2023-04-03T22:39:53Z

 }

-void resource_quota_server(const CoreTestConfiguration& config) {
+TEST_P(ResourceQuotaTest, ResourceQuota) {


There seems to be some changes to the configuration, e.g. kNumCalls from 100 -> 8, resource quota from 5MiB to 1MiB. Are these intended?

Yeah, we'd turned off the test for epoll1 a few years ago, effectively removing most of our coverage. I tuned down the values but ensured we were hitting the coverage we needed.

yijiem · 2023-04-03T22:57:14Z

-      GPR_ASSERT(GRPC_CALL_OK == grpc_call_cancel(c, nullptr));
+  error = grpc_call_start_batch(c->c_call(), ops, static_cast<size_t>(op - ops),
+                                CqVerifier::tag(1), nullptr);
+  EXPECT_EQ(call_start_batch_expected_result, error);


Maybe EXPECT_EQ(error, call_start_batch_expected_result); (i.e. EXPECT_EQ(actual, expected) seems more natural?)

yijiem · 2023-04-03T22:58:57Z

+  error = grpc_call_start_batch(c->c_call(), ops, static_cast<size_t>(op - ops),
+                                CqVerifier::tag(1), nullptr);
+  EXPECT_EQ(call_start_batch_expected_result, error);
+  if (error == GRPC_CALL_OK) {


The original code this seems to be if (expectation == GRPC_CALL_OK) {. Not sure if there is any difference though.

i think it's the same, and that this is more clear

yijiem · 2023-04-03T23:12:44Z

-  cqv.Verify();
+  CoreEnd2endTest::IncomingStatusOnClient server_status;
+  CoreEnd2endTest::IncomingMetadata server_initial_metadata;
+  c.NewBatch(1)


On line 48 server's batch 102 has a RecvCloseOnServer op, but there is no SendCloseFromClient op here. Does it get send internally?

it does not, but in this test we rely on the server to finish the call without the client doing so

yijiem · 2023-04-03T23:21:36Z

-
-void no_op_pre_init(void) {}
+namespace grpc_core {
+TEST_P(CoreEnd2endTest, NoOp) {}


drfloob · 2023-04-11T01:56:09Z

This PR disabled the EventEngine experiment tests, which were previously enabled for all h2_local_ipv4 end2end tests. We need a way to enable a subset of the core end2end tests incrementally, or risk not having any end2end tests enabled until all edge cases are ironed out in all EventEngine implementations.

I see a few options with this new test structure:

Turn the core_end2end_tests target into a library, and have multiple grpc_cc_tests depend on it with varying test filters and environment variables enabled.
define a separate end2end test build target with many/all of the same test source files, but a different main file that only enables a subset of them.
hack the current end2end_test_main.cc file to conditionally disable most tests if the experiments are not enabled, via IsEventEngineClientEnabled().
Add a notion of experiment compatibility to CoreTestConfiguration.

(1) seems the most palatable to me because it keeps all test configurations declarative.

The other options move a declarative experiment configuration into code, which is not great. Option (3) can easily become a mess of conditions around every test (or a map of tests to experiments, like we had before this PR). Option (4) seems the next most palatable to me, but the code will contain a lot of boilerplate ConfigQuery().ExcludeExperiments({GRPC_EXPERIMENT_EVENT_ENGINE_CLIENT}).

The previous configuration which was nuked:

    "h2_local_ipv4": _fixture_options(
        secure = True,
        dns_resolver = False,
        _platforms = ["linux", "mac", "posix"],
        tags = ["requires-net:ipv4", "requires-net:loopback", "event_engine_client"],
    ),

ctiller · 2023-04-11T13:16:49Z

Messaged you on chat, but to repeat here:

I think (3) via feature flags, or (4) - I don't want to return to a place where we're managing 40-ish different binaries each with customized main functions as implied by (1) or (2).

As much as possible we should choose to machine generate our build configuration and keep customization points in code.

It already started hitting the limit resulting in continuous failure. #32603 is believed to contribute to this time increase but let's bump it first and visit this issue later.

Implement listeners, connection, endpoints for `FuzzingEventEngine`. Allows the fuzzer to select write sizes and delays, connection delays, and port assignments. I made a few modifications to the test suite to admit this event engine to pass the client & server tests: 1. the test factories return shared_ptr<> to admit us to return the same event engine for both the oracle and the implementation - necessary because FuzzingEventEngine forms a closed world of addresses & ports. 2. removed the WaitForSingleOwner calls - these seem unnecessary, and we don't ask our users to do this - tested existing linux tests 1000x across debug, asan, tsan with this change Additionally, the event engine overrides the global port picker logic so that port assignments are made by the fuzzer too. This PR is a step along a longer journey, and has some outstanding brethren PR's, and some follow-up work: * grpc#32603 will convert all the core e2e tests into a more malleable form * we'll then use grpc#32667 to turn all of these into fuzzers * finally we'll integrate this into that work and turn all core e2e tests into fuzzers over timer & callback reorderings and io size/spacings --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>

Discovered testing grpc#32603

Notes: - `+trace` fixtures haven't run since 2016, so they're disabled for now (grpc@7ad2d0b#diff-780fce7267c34170c1d0ea15cc9f65a7f4b79fefe955d185c44e8b3251cf9e38R76) - all current fixtures define `FEATURE_MASK_SUPPORTS_AUTHORITY_HEADER` and hence `authority_not_supported` has not been run in years - deleted - bad_hostname similarly hasn't been triggered in a long while, so deleted - load_reporting_hook has never been enabled, so deleted (https://github.com/grpc/grpc/blame/f23fb4cf3114787806c330985c8fb3213597a09b/test/core/end2end/generate_tests.bzl#L145-L148) - filter_latency & filter_status_code rely on global variables and so don't convert particularly cleanly - and their value seems marginal, so deleted --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>

It already started hitting the limit resulting in continuous failure. grpc#32603 is believed to contribute to this time increase but let's bump it first and visit this issue later.

Implement listeners, connection, endpoints for `FuzzingEventEngine`. Allows the fuzzer to select write sizes and delays, connection delays, and port assignments. I made a few modifications to the test suite to admit this event engine to pass the client & server tests: 1. the test factories return shared_ptr<> to admit us to return the same event engine for both the oracle and the implementation - necessary because FuzzingEventEngine forms a closed world of addresses & ports. 2. removed the WaitForSingleOwner calls - these seem unnecessary, and we don't ask our users to do this - tested existing linux tests 1000x across debug, asan, tsan with this change Additionally, the event engine overrides the global port picker logic so that port assignments are made by the fuzzer too. This PR is a step along a longer journey, and has some outstanding brethren PR's, and some follow-up work: * #32603 will convert all the core e2e tests into a more malleable form * we'll then use #32667 to turn all of these into fuzzers * finally we'll integrate this into that work and turn all core e2e tests into fuzzers over timer & callback reorderings and io size/spacings --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>

Discovered testing #32603

Notes: - `+trace` fixtures haven't run since 2016, so they're disabled for now (7ad2d0b#diff-780fce7267c34170c1d0ea15cc9f65a7f4b79fefe955d185c44e8b3251cf9e38R76) - all current fixtures define `FEATURE_MASK_SUPPORTS_AUTHORITY_HEADER` and hence `authority_not_supported` has not been run in years - deleted - bad_hostname similarly hasn't been triggered in a long while, so deleted - load_reporting_hook has never been enabled, so deleted (https://github.com/grpc/grpc/blame/f23fb4cf3114787806c330985c8fb3213597a09b/test/core/end2end/generate_tests.bzl#L145-L148) - filter_latency & filter_status_code rely on global variables and so don't convert particularly cleanly - and their value seems marginal, so deleted --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>

It already started hitting the limit resulting in continuous failure. #32603 is believed to contribute to this time increase but let's bump it first and visit this issue later.

ctiller and others added 11 commits March 3, 2023 16:49

prototype

38afec7

Merge branch 'master' of https://github.com/grpc/grpc into leaping-li…

de85d5d

…zards

round 1

7440987

Automated change: Fix sanity tests

1e95d5e

Merge pull request #1147 from ctiller/create-pull-request/patch-7440987

20e8504

Automated fix for refs/heads/leaping-lizards

Automated change: Fix sanity tests

491bdc0

Merge pull request #1148 from ctiller/create-pull-request/patch-20e8504

2225fa7

Automated fix for refs/heads/leaping-lizards

ping-pong-streaming

23cb861

Merge branch 'leaping-lizards' of github.com:ctiller/grpc into leapin…

b0551f5

…g-lizards

cancel-after-accept

41c91d7

move more fixtures in

5c7576b

github-actions Bot added lang/core lang/ObjC labels Mar 11, 2023

cancel-after-client-done

3ddde70

grpc-checks Bot added bloat/none per-call-memory/neutral per-channel-memory/neutral labels Mar 11, 2023

ctiller added 13 commits March 11, 2023 16:50

empty-batch, cancel-after-client-invoke

fedca24

cancel-before-invoke

74ae7b2

more tests, more fixtures

4468893

remove abandoned tests

ccecaef

hpack-size

c0e6d02

binary-metadata

55c2c4b

progress

eebe0b3

trailing-metadata

8b1cf6b

negative-deadline

ce00cbf

fixture movements

63a402d

fixture movements

cb82b97

fix

a213a5b

fixtures

5fbf34f

grpc-checks Bot added the bloat/none label Apr 1, 2023

ctiller and others added 9 commits March 31, 2023 23:03

no lib

020f5f1

provide an override hook to add fixtures

a2f1475

Automated change: Fix sanity tests

5a2dc40

Merge pull request #1192 from ctiller/create-pull-request/patch-a2f1475

be02f19

Automated fix for refs/heads/leaping-lizards

another attempt for internal

5a003d6

Merge branch 'leaping-lizards' of github.com:ctiller/grpc into leapin…

312f706

…g-lizards

fix-dep

c706d7b

Automated change: Fix sanity tests

85a1889

Merge pull request #1193 from ctiller/create-pull-request/patch-c706d7b

0fae96c

Automated fix for refs/heads/leaping-lizards

ctiller added the kokoro:force-run label Apr 2, 2023

grpc-kokoro removed the kokoro:force-run label Apr 2, 2023

yijiem approved these changes Apr 3, 2023

View reviewed changes

ctiller added 2 commits April 3, 2023 20:10

review-feedback

2bbd824

x

b0bf74e

ctiller removed the disposition/Needs Internal Changes label Apr 4, 2023

ctiller merged commit 724441d into grpc:master Apr 4, 2023

copybara-service Bot added the imported Specifies if the PR has been imported to the internal repository label Apr 4, 2023

veblush mentioned this pull request Apr 13, 2023

[Test] Bump the timeout of macos/grpc_basictests_c_cpp to 4h #32866

Merged

Uh oh!

Conversation

ctiller commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

drfloob commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctiller commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ctiller commented Mar 11, 2023 •

edited

Loading

drfloob commented Apr 11, 2023 •

edited

Loading