GridFS download test reenablement #1991
+167
−49
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,10 +17,18 @@ | |
| package com.mongodb.client.unified; | ||
|
|
||
| import com.mongodb.ClusterFixture; | ||
| import com.mongodb.lang.Nullable; | ||
| import org.bson.BsonArray; | ||
| import org.bson.BsonDocument; | ||
| import org.bson.BsonInt32; | ||
| import org.bson.BsonValue; | ||
| import org.bson.diagnostics.Logger; | ||
| import org.bson.diagnostics.Loggers; | ||
| import org.opentest4j.AssertionFailedError; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.function.Function; | ||
| import java.util.function.Supplier; | ||
|
|
@@ -39,6 +47,8 @@ | |
| import static java.lang.String.format; | ||
|
|
||
| public final class UnifiedTestModifications { | ||
| private static final Logger LOGGER = Loggers.getLogger("UnifiedTestModifications"); | ||
|
|
||
| public static void applyCustomizations(final TestDef def) { | ||
|
|
||
| // change-streams | ||
|
|
@@ -104,10 +114,6 @@ public static void applyCustomizations(final TestDef def) { | |
| .test("client-side-operations-timeout", "timeoutMS behaves correctly for tailable awaitData cursors", | ||
| "apply maxAwaitTimeMS if less than remaining timeout"); | ||
|
|
||
| def.skipJira("https://jira.mongodb.org/browse/JAVA-5491") | ||
| .when(() -> !serverVersionLessThan(8, 3)) | ||
| .test("client-side-operations-timeout", "operations ignore deprecated timeout options if timeoutMS is set", | ||
|
|
@@ -320,6 +326,15 @@ public static void applyCustomizations(final TestDef def) { | |
| def.skipJira("https://jira.mongodb.org/browse/JAVA-5689") | ||
| .file("gridfs", "gridfs-deleteByName") | ||
| .file("gridfs", "gridfs-renameByName"); | ||
| def.transform("JAVA-5839: Bump blocking/timeout to avoid CI latency failures", | ||
| (entitiesArray, definition) -> { | ||
| findAndSetInt(entitiesArray, "client.uriOptions.timeoutMS", 250); | ||
| findAndSetInt(definition.getArray("operations"), | ||
| "arguments.failPoint.data.blockTimeMS", 200); | ||
| }) | ||
| .test("client-side-operations-timeout", | ||
| "timeoutMS behaves correctly for GridFS download operations", | ||
| "timeoutMS applied to entire download, not individual parts"); | ||
|
|
||
| // Skip all rawData based tests | ||
| def.skipJira("https://jira.mongodb.org/browse/JAVA-5830 rawData support only added to Go and Node") | ||
|
|
@@ -505,30 +520,69 @@ public static void applyCustomizations(final TestDef def) { | |
| .file("unified-test-format/tests/valid-fail", "operator-matchAsDocument"); | ||
| } | ||
|
|
||
| /** | ||
| * Searches each document in {@code array} for a nested int field specified | ||
| * by a dot-separated {@code path}, and replaces it with {@code newValue}. | ||
| * Logs each replacement. Silently skips documents where the path does not | ||
| * exist or the intermediate keys are absent. | ||
| * | ||
| * <p>Example: {@code findAndSetInt(entitiesArray, "client.uriOptions.timeoutMS", 250)} | ||
| * walks each element looking for {@code element.client.uriOptions.timeoutMS}.</p> | ||
| * | ||
| * @param array the array to search | ||
| * @param path dot-separated path to an int field | ||
| * @param newValue the replacement value | ||
| */ | ||
| static void findAndSetInt(final BsonArray array, final String path, final int newValue) { | ||
|
Collaborator
There was a problem hiding this comment. can we generalise this method for all numbers? and use
Collaborator
There was a problem hiding this comment. Can we refactor this to include the reason ? this will make the log statement below ( Ex: String reason = "JAVA-5839: Bump blocking/timeout to avoid CI latency failures";
def.transform(reason,
(entitiesArray, definition) -> {
findAndSetInt(reason, entitiesArray, "client.uriOptions.timeoutMS", 250);
findAndSetInt(reason, definition.getArray("operations"), "arguments.failPoint.data.blockTimeMS", 200);
})Or infer it from // TestDef ...
public void applyTransformations(final BsonArray entitiesArray, final BsonDocument definition) {
for (Transformation t : transformations) {
t.transformer.transform(t.reason, entitiesArray, definition);
}
}
// lambda
def.transform("JAVA-5839: Bump blocking/timeout to avoid CI latency failures",
(reason, entitiesArray, definition) -> {
findAndSetInt(reason, entitiesArray, "client.uriOptions.timeoutMS", 250);
findAndSetInt(reason, definition.getArray("operations"), "arguments.failPoint.data.blockTimeMS", 200);
}) |
||
| String[] segments = path.split("\\."); | ||
| for (BsonValue element : array) { | ||
| if (!element.isDocument()) { | ||
| continue; | ||
| } | ||
| BsonDocument current = element.asDocument(); | ||
| boolean found = true; | ||
| for (int i = 0; i < segments.length - 1; i++) { | ||
| if (current.containsKey(segments[i]) && current.get(segments[i]).isDocument()) { | ||
| current = current.getDocument(segments[i]); | ||
| } else { | ||
| found = false; | ||
| break; | ||
| } | ||
| } | ||
| String leafKey = segments[segments.length - 1]; | ||
| if (found && current.containsKey(leafKey) && current.get(leafKey).isInt32()) { | ||
|
Collaborator
There was a problem hiding this comment. The transformation is skipped if the value is |
||
| int oldValue = current.getInt32(leafKey).getValue(); | ||
| LOGGER.info(format(" %s: %d -> %d", leafKey, oldValue, newValue)); | ||
| current.put(leafKey, new BsonInt32(newValue)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private UnifiedTestModifications() { | ||
| } | ||
|
|
||
| public static TestDef testDef(final String directory, final String fileDescription, final String testDescription, | ||
| final boolean reactive, final UnifiedTest.Language language) { | ||
| return new TestDef(directory, fileDescription, testDescription, reactive, language); | ||
| } | ||
|
|
||
| public static final class TestDef { | ||
|
|
||
| private final String directory; | ||
| private final String fileDescription; | ||
| private final String testDescription; | ||
| private final boolean reactive; | ||
| private final UnifiedTest.Language language; | ||
|
|
||
| private final List<Modifier> modifiers = new ArrayList<>(); | ||
| private final List<TestTransformer> transformers = new ArrayList<>(); | ||
| private Function<Throwable, Boolean> matchesThrowable; | ||
|
|
||
| private TestDef(final String directory, final String fileDescription, final String testDescription, | ||
| final boolean reactive, final UnifiedTest.Language language) { | ||
| this.directory = assertNotNull(directory); | ||
| this.fileDescription = assertNotNull(fileDescription); | ||
| this.testDescription = assertNotNull(testDescription); | ||
| this.reactive = reactive; | ||
| this.language = assertNotNull(language); | ||
| } | ||
|
|
@@ -538,9 +592,9 @@ public String toString() { | |
| return "TestDef{" | ||
| + "modifiers=" + modifiers | ||
| + ", reactive=" + reactive | ||
| + ", testDescription='" + testDescription + '\'' | ||
| + ", fileDescription='" + fileDescription + '\'' | ||
| + ", directory='" + directory + '\'' | ||
| + '}'; | ||
| } | ||
|
|
||
|
|
@@ -614,6 +668,34 @@ public TestApplicator retryReactive(final String reason) { | |
| .when(this::isReactive); | ||
| } | ||
|
|
||
| /** | ||
| * Registers a transformation that mutates the test's entity and | ||
| * definition data before execution. The reason is logged when the | ||
| * transformation is registered for a matching test. | ||
| * | ||
|
rozza marked this conversation as resolved.
|
||
| * @param reason why the transformation is needed | ||
| * @param transformer the transformation to apply | ||
| */ | ||
| public TestApplicator transform(final String reason, final TestTransformer transformer) { | ||
| return new TestApplicator(this, reason, transformer); | ||
| } | ||
|
|
||
| /** | ||
| * Applies all registered transformations to the test data. | ||
| */ | ||
| public void applyTransformations(final BsonArray entitiesArray, final BsonDocument definition) { | ||
| for (TestTransformer transformer : transformers) { | ||
| transformer.transform(entitiesArray, definition); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if any transformations have been registered. | ||
| */ | ||
| public boolean hasTransformations() { | ||
| return !transformers.isEmpty(); | ||
| } | ||
|
|
||
| public TestApplicator modify(final Modifier... modifiers) { | ||
| return new TestApplicator(this, null, modifiers); | ||
| } | ||
|
|
@@ -648,18 +730,34 @@ public static final class TestApplicator { | |
|
|
||
| private final List<Modifier> modifiersToApply; | ||
| private Function<Throwable, Boolean> matchesThrowable; | ||
| @Nullable | ||
| private final TestTransformer transformer; | ||
| @Nullable | ||
| private final String reason; | ||
|
|
||
| private TestApplicator( | ||
| final TestDef testDef, | ||
| @Nullable final String reason, | ||
| final Modifier... modifiersToApply) { | ||
| this.testDef = testDef; | ||
| this.reason = reason; | ||
| this.modifiersToApply = Arrays.asList(modifiersToApply); | ||
| this.transformer = null; | ||
| if (this.modifiersToApply.contains(SKIP) || this.modifiersToApply.contains(RETRY)) { | ||
| assertNotNull(reason); | ||
| } | ||
| } | ||
|
|
||
| private TestApplicator( | ||
| final TestDef testDef, | ||
| final String reason, | ||
| final TestTransformer transformer) { | ||
| this.testDef = testDef; | ||
| this.reason = assertNotNull(reason); | ||
| this.modifiersToApply = Collections.emptyList(); | ||
| this.transformer = assertNotNull(transformer); | ||
| } | ||
|
|
||
| private TestApplicator onMatch(final boolean match) { | ||
| matchWasPerformed = true; | ||
| if (precondition != null && !precondition.get()) { | ||
|
|
@@ -668,83 +766,88 @@ private TestApplicator onMatch(final boolean match) { | |
| if (match) { | ||
| this.testDef.modifiers.addAll(this.modifiersToApply); | ||
| this.testDef.matchesThrowable = this.matchesThrowable; | ||
| if (this.transformer != null) { | ||
| LOGGER.info("Registered transformation for test [" | ||
| + testDef.testDescription + "]: " + reason); | ||
| this.testDef.transformers.add(this.transformer); | ||
| } | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Applies to all tests in directory. | ||
| * | ||
| * @param directory the directory name | ||
| * @return this | ||
| */ | ||
| public TestApplicator directory(final String directory) { | ||
| boolean match = (directory).equals(testDef.directory); | ||
| return onMatch(match); | ||
| } | ||
|
|
||
| /** | ||
| * Applies to all tests in file under the directory. | ||
| * | ||
| * @param directory the directory name | ||
| * @param fileDescription the test file's "description" field | ||
| * @return this | ||
| */ | ||
| public TestApplicator file(final String directory, final String fileDescription) { | ||
| boolean match = (directory).equals(testDef.directory) | ||
| && fileDescription.equals(testDef.fileDescription); | ||
| return onMatch(match); | ||
| } | ||
|
|
||
| /** | ||
| * Applies to the test where directory, fileDescription, and testDescription match. | ||
| * | ||
| * @param directory the directory name | ||
| * @param fileDescription the test file's "description" field | ||
| * @param testDescription the individual test's "description" field | ||
| * @return this | ||
| */ | ||
| public TestApplicator test(final String directory, final String fileDescription, final String testDescription) { | ||
| boolean match = testDef.directory.equals(directory) | ||
| && testDef.fileDescription.equals(fileDescription) | ||
| && testDef.testDescription.equals(testDescription); | ||
| return onMatch(match); | ||
| } | ||
|
|
||
| /** | ||
| * Utility method: emit replacement to standard out. | ||
| * | ||
| * @param directory the directory name | ||
| * @param fragment the substring to check in the test "description" field | ||
| * @return this | ||
| */ | ||
| public TestApplicator testContains(final String directory, final String fragment) { | ||
| boolean match = (directory).equals(testDef.directory) | ||
| && testDef.testDescription.contains(fragment); | ||
| if (match) { | ||
| System.out.printf( | ||
| "!!! REPLACE %s WITH: .test(\"%s\", \"%s\", \"%s\")%n", | ||
| fragment, | ||
| testDef.directory, | ||
| testDef.fileDescription, | ||
| testDef.testDescription); | ||
| } | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Utility method: emit file info to standard out | ||
| * | ||
| * @param directory the directory name | ||
| * @param testDescription the individual test's "description" field | ||
| * @return this | ||
| */ | ||
| public TestApplicator debug(final String directory, final String testDescription) { | ||
| boolean match = testDef.testDescription.equals(testDescription); | ||
| if (match) { | ||
| System.out.printf( | ||
| "!!! ADD: \"%s\", \"%s\", \"%s\"%n", | ||
| testDef.directory, testDef.fileDescription, testDescription); | ||
| } | ||
| return this; | ||
| } | ||
|
|
@@ -788,6 +891,16 @@ public TestApplicator whenFailureContains(final String messageFragment) { | |
|
|
||
| } | ||
|
|
||
| /** | ||
| * A transformation that mutates the test's entity array and/or definition | ||
| * before execution. Used to adjust spec test values (e.g., timeouts) that | ||
| * are too tight for CI environments. | ||
| */ | ||
| @FunctionalInterface | ||
| public interface TestTransformer { | ||
| void transform(BsonArray entitiesArray, BsonDocument definition); | ||
| } | ||
|
|
||
| public enum Modifier { | ||
| /** | ||
| * Reactive only. | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a value in asserting the expected original value? this will make sure if the upstream test changes to a more favourable value maybe the transformation is no longer needed ?