Rework YamlIntegrationTests to not use inheritance #3083
Merged
Conversation
This file contains 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 just adds one config, and copies one test using it.
This fixes a spotbugs error. MultiServerConnectionFactory still has a problem of not closing, in theory, and that will probably need to be resolved too, as that gets brought into "production" code.
This is basically the same as JDBCInProcessYamlIntegrationTests except we don't wrap the exception in the afterAll
Just moving the code into a different class
This required running two servers with separate ports. I could have had them try to share an external server, but this seems simpler, especially if we later have configs that run with extra versions of the server.
Not everything is supported JDBC yet, so we need the ability to skip some tests. It seemed clearer to do it from the config side, so you can easily see what is not supported for a config. The configs that are not supported for a test will be visible when you run the test.
This allows deleting all the other implementations. YamlTestBase still exists as it is used for the supported_version tests
A lot of times (I assume) people will want to test changes to the relational core, or record-layer core, and will want faster tests then running all of the configs. This will allow easily doing so in intelliJ with a new `quickTest` target when you got o run a test. The quickTest target is not run as part of `./gradlew build`, it has to be explicitly requested, so we won't get redundant test runs in our CI.
This allows other projects to use @yamltest exclude some of their tests from some configs.
now unused
Result of fdb-record-layer-pr on Linux CentOS 7
|
MultiServerConnectionFactory had a conflict on getNewConnection that was fairly straightforward ExternalServer had a conflict because main optionally redirects output, and this branch changed the arguments to the jar. To avoid conflicts in the redirect output (because there may now be multiple concurrent servers), it appends the port to the filename.
Result of fdb-record-layer-pr on Linux CentOS 7
|
We now call unwrap, so that needs to work. We don't call anything else though, which makes Mockito great.
ohadzeliger
requested changes
Feb 4, 2025
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/ExcludeYamlTestConfig.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlTest.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlTestExtension.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlTestExtension.java
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlTestExtension.java
Show resolved
Hide resolved
.../src/main/java/com/apple/foundationdb/relational/yamltests/MultiServerConnectionFactory.java
Show resolved
Hide resolved
...sts/src/main/java/com/apple/foundationdb/relational/yamltests/configs/MultiServerConfig.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/ExternalServer.java
Outdated
Show resolved
Hide resolved
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/server/ExternalServer.java
Outdated
Show resolved
Hide resolved
Also, remove ExternalServer.SERVER_PORT, fixing the one incorrect usage of it.
I adedd a reason to explain why the test is excluded, and in doing so, discovered I accidentally marked a test as excluded, and shouldn't have (presumably testing @ExcludedTestConfig, and accidentally committed it).
In order to support this, I also changed MultiServerConfig to extend JDBCInProcessConfig, so that the tests don't have to list both when excluding.
This conflicted on YamlTestBase because main adds getAdditionalOptions, and this branch changes YamlRunner.run() to not throw exceptions. It also conflicted on MultiServerConfig, because the merge process tried to take in MultiServerForceContinuationsTests. There were also new usages of YamlRunner.run(), and for those I added Map.of() for the additional options. This completely drops the coverage from MultiServerForceContinuationsTests, I will add that back, and probably additional coverage in a second commit.
This adds a new config to run with FORCE_CONTINUATIONS=true, and runs it with both combinations of mixed mode. This required reworking @ExcludeYamlTestConfig to support the new permutations.
ohadzeliger
reviewed
Feb 6, 2025
...sts/src/main/java/com/apple/foundationdb/relational/yamltests/configs/ConfigWithOptions.java
Show resolved
Hide resolved
ohadzeliger
reviewed
Feb 6, 2025
yaml-tests/src/main/java/com/apple/foundationdb/relational/yamltests/YamlTestExtension.java
Show resolved
Hide resolved
ohadzeliger
approved these changes
Feb 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The inheritance tree makes it hard to add, or mix configurations. It also makes it impossible to have dynamically generated configurations (e.g. run mixed-mode with all of a bunch of different old versions). At some point it would also make it cumbersome, as all the tests have to live in one place.
The key thing here is the introduction of
@YamlTestannotation which provides contexts for@TestTemplate, and is configured to run with a bunch of differentYamlTestConfigs.You may find this easier to review commit-by-commit. Or maybe not. The commits are pretty sensible, although a bunch of the classes don't get javadoc until the very end.
I left
YamlTestBase, so I think this should be backwards-compatible, but I expect to remove that soon.