Remove yaml-tests dependency on fdb-record-layer-core test code #3252
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
By removing this dependency we make it easier for others to use the yaml testing framework.
If someone wants to use YamlTestExtension, they'll have to add it with @ExtendWith
This re-enables the debugger in our tests, but allows yaml-tests to be used elsewhere without bringing in a dependency on the debugger.
ScottDugas
added
testing improvement
ScottDugas
added a commit
to ScottDugas/fdb-record-layer
that referenced
this pull request
Mar 17, 2025
When publishing to JUnit xml style files there is no hierarchy, so
some tools will just show the class name and description, making it
hard to tell what tests were run, or failed. If YamlTestExtension
is used in an environment where that is the case this can be used
to have the output include the method name. For example:
- showcasingTests(Embedded)
- showcasingTests(MultiServer (Embedded then !current_version))
...
Instead of
- YamlIntegrationTests
- showcasingTests(Runner)
- Embedded
- MultiServer (Embedded then !current_version)
I thought about making this configurable via system property, but
I think having it configured by a constructor parameter aligns more
with PRs FoundationDB#3252 and FoundationDB#3251
normen662
requested changes
Mar 18, 2025
| @@ -179,13 +174,13 @@ private void executeInternal(@Nonnull final YamlConnection connection, boolean c | |||
| // can result in the explain plan being put into the plan cache, so running without the debugger | |||
| // can pollute cache and thus interfere with the explain's results | |||
| Integer finalMaxRows = maxRows; | |||
| runWithDebugger(() -> executor.execute(connection, null, queryConfig, checkCache, finalMaxRows)); | |||
| executor.execute(connection, null, queryConfig, checkCache, finalMaxRows); | |||
There was a problem hiding this comment.
This disables all sanity checks within the planner. See Debugger.isSane()
| } else if (QueryConfig.QUERY_CONFIG_EXPLAIN.equals(queryConfig.getConfigName()) || QueryConfig.QUERY_CONFIG_EXPLAIN_CONTAINS.equals(queryConfig.getConfigName())) { | ||
| Assert.that(!queryIsRunning, "Explain test should not be intermingled with query result tests"); | ||
| // ignore debugger configuration, always set the debugger for explain, so we can always get consistent | ||
| // results | ||
| Integer finalMaxRows1 = maxRows; | ||
| runWithDebugger(() -> executor.execute(connection, null, queryConfig, checkCache, finalMaxRows1)); | ||
| executor.execute(connection, null, queryConfig, checkCache, finalMaxRows1); |
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.
This changes the main code for
yaml-teststo not depend on the test configuration offdb-record-layer-core, allowing it to be more easily used in other projects.yaml-testshad the following test dependencies:TestHelpers.assertThrows- Replaced with junit'sAssertions.assertThrows. We should probably do this across the code base, but I did not as part of this PR.TestHelpers.DangerousRunnable- this was fairly easy to remove as it was only used byQueryCommand.runWithDebugger. I think this is unnecessary as we set the debugger globally.DebuggerWithSymbolTables- I believe this is only used to debug the behavior of the planner, which doesn't seem like something people should be testing outside, so moving this to the test code seemed reasonable. In order to accomplish this, I added a new extension:YamlTestWithDebuggingExtensionthat extendsYamlTestExtensionbut initializes the debugger in thebeforeAll. In order to use this, I moved@YamlTestthe test code. This seems like a good idea anyways, as that adds theMixedModecategory.With the above changes I removed
YamlTest.Runner, and changedYamlRunnerto be an acceptable parameter to the tests.