perf(test): Speed up orca-sql tests #3827
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
When overriding setupSpec, Spock runs both the parent (super.setupSpec) and the subclass setupSpec (in that order). This means that for the PgSQL tests, we're creating a MySQL database then immediately discarding it to replace it with a PgSQL database. As creating the database spins up a container and is resonably slow, lightly refactor this so that we don't do this extra work.
There's a lot of coupling between PipelineExecutionRepositoryTck and its subclasses that causes initialization order dependencies. In order to make it easier to make changes (such as changing what fields are updated in setup vs setupSpec), refactor this so that each subclass is fully responsible for initialization and we can control the order. The main issue here is that the super class was storing and initializing the ExecutionRepository, but we can't actually create the ExecutionRepository until we've created the persistence store, which is owned by the implementations. So we were subtly depending on each subclass having already initialized its persistence store before we create the ExecutionRepository. Now it's up to each subclass ot handle this order-dependence and expose a method to get the ExecutionRepository.
Given that we share the actual persistence between tests, there's not much benefit to creating a new ExecutionRepository for each test. Speed the tests up by re-using it between tests.
The previous repository is only used in the Jedis tests; remove it from the base class so we can avoid creating it and cleaning it up after every test.
KathrynLewis
pushed a commit
to KathrynLewis/orca
that referenced
this pull request
Jan 31, 2021
* perf(test): Speed up PgSQL tests When overriding setupSpec, Spock runs both the parent (super.setupSpec) and the subclass setupSpec (in that order). This means that for the PgSQL tests, we're creating a MySQL database then immediately discarding it to replace it with a PgSQL database. As creating the database spins up a container and is resonably slow, lightly refactor this so that we don't do this extra work. * refactor(test): Push some logic down from execution repository tests There's a lot of coupling between PipelineExecutionRepositoryTck and its subclasses that causes initialization order dependencies. In order to make it easier to make changes (such as changing what fields are updated in setup vs setupSpec), refactor this so that each subclass is fully responsible for initialization and we can control the order. The main issue here is that the super class was storing and initializing the ExecutionRepository, but we can't actually create the ExecutionRepository until we've created the persistence store, which is owned by the implementations. So we were subtly depending on each subclass having already initialized its persistence store before we create the ExecutionRepository. Now it's up to each subclass ot handle this order-dependence and expose a method to get the ExecutionRepository. * perf(test): Share execution repository among tests Given that we share the actual persistence between tests, there's not much benefit to creating a new ExecutionRepository for each test. Speed the tests up by re-using it between tests. * perf(test): Remove previous repository from base test The previous repository is only used in the Jedis tests; remove it from the base class so we can avoid creating it and cleaning it up after every test. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.
Reduces the time to run
:orca-sql:testfrom ~2m30s to ~1m40s.perf(test): Speed up PgSQL tests
When overriding setupSpec, Spock runs both the parent (super.setupSpec) and the subclass setupSpec (in that order). This means that for the PgSQL tests, we're creating a MySQL database then immediately discarding it to replace it with a PgSQL database.
As creating the database spins up a container and is resonably slow, lightly refactor this so that we don't do this extra work.
refactor(test): Push some logic down from execution repository tests
There's a lot of coupling between PipelineExecutionRepositoryTck and its subclasses that causes initialization order dependencies. In order to make it easier to make changes (such as changing what fields are updated in setup vs setupSpec), refactor this so that each subclass is fully responsible for initialization and we can control the order.
The main issue here is that the super class was storing and initializing the ExecutionRepository, but we can't actually create the ExecutionRepository until we've created the persistence store, which is owned by the implementations. So we were subtly depending on each subclass having already initialized its persistence store before we create the ExecutionRepository.
Now it's up to each subclass ot handle this order-dependence and expose a method to get the ExecutionRepository.
perf(test): Share execution repository among tests
Given that we share the actual persistence between tests, there's not much benefit to creating a new ExecutionRepository for each test. Speed the tests up by re-using it between tests.
perf(test): Remove previous repository from base test
The previous repository is only used in the Jedis tests; remove it from the base class so we can avoid creating it and cleaning it up after every test.