Refactor connection management in bqjdbc tests #186
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
fzakaria
reviewed
Dec 26, 2023
src/test/java/net/starschema/clouddb/jdbc/BQForwardOnlyResultSetFunctionTest.java
Show resolved
Hide resolved
src/test/java/net/starschema/clouddb/jdbc/BQForwardOnlyResultSetFunctionTest.java
Outdated
Show resolved
Hide resolved
src/test/java/net/starschema/clouddb/jdbc/BQForwardOnlyResultSetFunctionTest.java
Show resolved
Hide resolved
Use common `ConnectionFromResources.connect` method instead of a bespoke `NewConnection` method. Also, use instance variable/test case scoped connections and result sets instead of static/suite scoped ones. This should help enable test parallelization later. Local testing shows no reliable increase in `BQScrollableResultSetFunctionTest` total runtime (~30 seconds both before and after change).
Use common `ConnectionFromResources.connect` method instead of a bespoke `NewConnection` method. Also, use instance variable/test case scoped connections and result sets instead of static/suite scoped ones. This should help enable test parallelization later. Local testing shows no reliable increase in runtime (~30 seconds). This was eased by the introduction of a second BQ connection with legacy SQL disabled (`standardSqlConnection`). Not every test uses both connections, so this is a little wasteful and risks introducing some unreliability, but it isolates test better than continually closing and reopening a static connection.
Use common `ConnectionFromResources.connect` method instead of a bespoke `NewConnection` method. Also, use instance variable/test case scoped connections and result sets instead of static/suite scoped ones. This should help enable test parallelization later. Local testing shows no reliable increase in runtime (~30 seconds).
Use common `ConnectionFromResources.connect` method instead of a bespoke `NewConnection` method. Also, use instance variable/test case scoped connections and result sets instead of static/suite scoped ones. This should help enable test parallelization later. Local testing shows no reliable increase in runtime (~12 seconds). This was eased by the introduction of a second BQ connection with legacy SQL disabled (`standardSqlConnection`). Not every test uses both connections, so this is a little wasteful and risks introducing some unreliability, but it isolates test better than continually closing and reopening a static connection.
Use common `ConnectionFromResources.connect` method instead of a bespoke `NewConnection` method. Also, use instance variable/test case scoped connections and result sets instead of static/suite scoped ones. This should help enable test parallelization later. Local testing shows no reliable increase in total runtime (~6 seconds both before and after change).
Now each @before method sets a single instance variable. This is to address code review comments.
goomrw
force-pushed
the
goomrw-test-connection-refactor
branch
from
bd122ed
to
e61041d
Compare
fzakaria
approved these changes
Jan 4, 2024
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.
Tests in bqjdbc use redundant but slightly different code to manage connections; for example, compare BQForwardOnlyResultSetFunctionTest.NewConnection with BQScrollableResultSetFunctionTest.NewConnection.
These methods also assign connections to static variables, which makes accessing them annoying and hampers test parallelization.
A previous change introduced a ConnectionFromResources helper class that can replace this duplicated connection management code with instance-level connections. This might slow down tests in the short term but should allow parallelization later.
Best reviewed by commit!