DRIVERS-1713 Add ignoreExtraEvents to expectedEventsForClient
#1153
Conversation
There was a problem hiding this comment.
What legacy change stream test(s) actually required this functionality? I have some recollection of discussing this with @kmahar and thought that the extra events were only relevant on sharded clusters and could be avoided if we limited testing to replica sets (as I believe most of the legacy tests already do).
| @@ -0,0 +1,15 @@ | |||
| description: "expectedEventsForClient-additionalProperties" | |||
There was a problem hiding this comment.
Based on existing test files, The correct filename and description for this should be expectedEventsForClient-eventMatch-type.
I'd also suggest creating a separate expectedEventsForClient-eventType-enum test that uses a string value not found in the schema's enum (e.g. "foo").
There was a problem hiding this comment.
Whoops, right. Fixed! Didn't add the -enum case because it's now a bool as per the other comment.
| matched against the observed events. Valid values are ``exact``, meaning | ||
| that observed events after all specified events have matched MUST cause a | ||
| test failure, and ``prefix``, meaning that observed events after all | ||
| specified events have been matched MUST NOT cause a test failure. |
There was a problem hiding this comment.
With respect to the Defy Augury mantra, is it likely that additional matching strategies will be needed down the line? If not, I think this might make more sense as an ignoreExtraEvents boolean that defaults to false.
If that change is made, the only necessary invalid test would be expectedEventsForClient-ignoreExtraEvents-type with a non-boolean value.
There was a problem hiding this comment.
Makes sense; I certainly see no needs for future matching modes, and it's hard to imagine what reasonable ones would even be. Updated.
|
Quite a few of the tests are written to implicitly rely on the prefix matching even without the variance in In the initial discussions that I was part of, restricting the tests to replica sets didn't come up as an option; the small additive change to the unified test spec to allow prefixes had general consensus as a reasonable path forward. With that as background I implemented DRIVERS-1713 as a semi-automated conversion rather than a rewrite. It might be possible to remove the need for the unified test change by updating those tests that fail under exact matching, either by filling out the full set of expected events or restricting them to replsets or both; that has the downsides of making the tests somewhat more brittle and reducing their coverage, as well as the extra engineering time that would take. |
|
@jmikola for reference we discussed this previously here: https://mongodb.slack.com/archives/G7U1QFDPE/p1643133976071300 by my count the majority of legacy tests (38 of 52) are currently run against sharded topologies, and it seems like any test that iterates a change stream (most/all of them, I think?) would be susceptible to the extra getMores issue, so it does seem like we would lose a lot of coverage if we stopped testing against sharded clusters |
|
Points noted. I just re-read that Slack thread (thanks for digging it up) and in light of that this seems like a reasonable change. I'm glad we don't have to worry about ignoring events within the sequence (as discussed in that thread). So I have no objections to this change, but my two comments above on the diff do apply. I wonder if a boolean flag makes more sense for this, unless we really think there will be a need to implement different matching strategies via the enum. |
| @@ -3281,6 +3287,8 @@ spec changes developed in parallel or during the same release cycle. | |||
| Change Log | |||
| ========== | |||
|
|
|||
| :2022-03-01: Add ``eventMatch`` field to ``expectedEventsForClient``. | |||
There was a problem hiding this comment.
| :2022-03-01: Add ``eventMatch`` field to ``expectedEventsForClient``. | |
| :2022-03-01: Add ``ignoreExtraEvents`` field to ``expectedEventsForClient``. |
abr-egn
force-pushed
the
DRIVERS-1713/eventMatch
branch
from
bac6217 to
c9c5e91
Compare
abr-egn
changed the title
eventMatch to expectedEventsForClientignoreExtraEvents to expectedEventsForClient
DRIVERS-1713
This introduces an optional
eventMatchfield to theexpectedEventsForClientstruct, allowing tests to specify whether the expected events listed need to precisely match against actual events (the current behavior and the default for the field) or whether the events listed can be a prefix of actual events. The latter behavior is needed to convert the change stream spec tests, which rely on prefix matching in order to allow for unavoidable variance ingetMorecommand count.The Rust driver has implemented this logic here, and I have a branch with the tests introduced in this PR copied in and passing here.