Clean up unified tests via skipping API#1551
Conversation
| * @param skip reason for skipping the test; must start with a Jira URL | ||
| */ | ||
| public Skip skipJira(final String skip) { | ||
| Assertions.assertTrue(skip.startsWith("https://jira.mongodb.org/browse/JAVA-")); |
There was a problem hiding this comment.
Can the skip be due to a DRIVER-XXX ticket? I think checking against https://jira.mongodb.org/browse/ is enough to make sure a valid JIRA link is provided
There was a problem hiding this comment.
I considered this, but I think we need to have something our team is tracking. If a good counterexample comes up, we can remove the constraint at the time?
| /** | ||
| * Test skipped if the test description contains the fragment as a substring | ||
| */ | ||
| public Skip testContains(final String dir, final String fragment) { |
There was a problem hiding this comment.
This should be used carefully as it has the potential risk of skipping new tests matching the fragment...
I accidentally used updateOne with sort option with contains which matched also BulkWrite updateOne with sort option
There was a problem hiding this comment.
I will add a note.
I think we should stop it from being used in non-dev, but this will be more work, since existing tests use this pattern.
| .file("server-discovery-and-monitoring", "connect with serverMonitoringMode=stream >=4.4"); | ||
|
|
||
| def.skipJira("https://jira.mongodb.org/browse/JAVA-4770") | ||
| .file("server-discovery-and-monitoring", "standalone-logging") |
There was a problem hiding this comment.
The standalone-logging here is the description of the test(s) https://github.com/mongodb/specifications/blob/master/source/server-discovery-and-monitoring/tests/unified/logging-standalone.json#L2 and not the JSON file name. The method defines the second parameter as the file https://github.com/mongodb/mongo-java-driver/pull/1551/files#diff-c703183de18b785f0df010e4d1dc57464f07946ae7e350533c507d4232801374R85
I was under the impression that If one wants to exclude all tests under logging-standalone.json I could call
def.skipJira("https://jira.mongodb.org/browse/JAVA-4770")
.file("server-discovery-and-monitoring", "logging-standalone")
// logging-standalone is the name of JSON file
// standalone-logging is the top description of all tests under logging-standalone.json In contrary in https://github.com/mongodb/mongo-java-driver/pull/1551/files#diff-20c83c94232058f9e9a6b35b2c2825e600f5dba0cbc5a4c64bfd0aa78053cd28R46
serverMonitoringMode is the actual JSON file name (which also matches its description)
There was a problem hiding this comment.
Having the description used as an identifier is unfortunate. Probably tests should have an id field that matches their file name.
Added param docs.
| /** | ||
| * Test skipped if the test description contains the fragment as a substring | ||
| */ | ||
| public Skip testContains(final String dir, final String fragment) { |
There was a problem hiding this comment.
Is there a use case where we want to filter by dir and test description as well? Thinking whether we should add an optional testDef.file parameter to the method?
There was a problem hiding this comment.
Let's add it if we run into it (hopefully we don't, since as you say the contains methods seem risky).
2 participants
This is a lightweight wrapper API for organizing assumptions ("skips") in unified tests. Offers a consistent and clearer mechanism for specifying why tests are being skipped.