Create Config Database test base class #19640
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
cgardens
commented
Nov 21, 2022
| class DatabaseConfigPersistenceTest extends BaseConfigDatabaseTest { | ||
|
|
||
| public static final String DEFAULT_PROTOCOL_VERSION = "0.2.0"; | ||
| protected static final StandardSourceDefinition SOURCE_GITHUB = new StandardSourceDefinition() |
There was a problem hiding this comment.
all of this noise is copied from BaseDatabaseConfigPersistenceTest. It will all get removed in the next PR which gets rid of DatabaseConfigPersistence entirely.
cgardens
commented
Nov 21, 2022
| statePersistence = new StatePersistence(database); | ||
| } | ||
|
|
||
| private void setupTestData() throws JsonValidationException, IOException { |
There was a problem hiding this comment.
this diff looks bigger than it is. this is just pulled up from lower down in the class.
benmoriceau
approved these changes
Nov 21, 2022
gosusnp
approved these changes
Nov 21, 2022
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.
What
DatabaseConfigPersistenceI realized that over time theBaseDatabaseConfigPersistenceTesthad been misused. This was making it so that test run much slower (because the full database migration was being run between each test) and that it was much harder to write test classes for checking our database queries because lots of boilerplate was being needlessly copied and pasted because people didn't understand how to use the base class.How
BaseDatabaseConfigPersistenceTestwithBaseConfigDatabaseTest. This is a much more opinionated abstraction that I think will be harder to misuse. The javadocs in the class explain it in detail, so I will not repeat it here.DatabaseConfigPersistencewhich relies on us splitting out our tests more clearly. Decided to separate out this change for clarity.Results
Time to run
airbyte-config:config-persistence:testbefore this change:Time to run
airbyte-config:config-persistence:testafter this change:A neat 55% speed increase!! 😄
Recommended reading order
BaseConfigDatabaseTest.java🚨 User Impact 🚨