refactor: change item seeding removal strategy #4264
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
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
2 times, most recently
from
475a741
to
b60abd4
Compare
dorner
reviewed
Apr 11, 2024
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
13 times, most recently
from
08a811d
to
e045c2f
Compare
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
from
e045c2f
to
d8c40f7
Compare
elasticspoon
commented
Apr 13, 2024
There was a problem hiding this comment.
Only real file with changes worth looking at, all the rest is adding tags to spec files.
elasticspoon
added a commit
to elasticspoon/human-essentials
that referenced
this pull request
Apr 13, 2024
As discussed here rubyforgood#4264, some tests modify global variables and expect them to be reset before each test. This makes it more difficult to move forward with removal of pre-seeding. This commit refactors certain tests to be less brittle to changes in global state and to rely less on assumptions on global state. Ex: not assuming that there are 0 records of a certain type when a test starts, etc.
elasticspoon
added a commit
to elasticspoon/human-essentials
that referenced
this pull request
Apr 13, 2024
As discussed here rubyforgood#4264, some tests modify global variables and expect them to be reset before each test. This makes it more difficult to move forward with removal of pre-seeding. This commit refactors certain tests to be less brittle to changes in global state and to rely less on assumptions on global state. Ex: not assuming that there are 0 records of a certain type when a test starts, etc.
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
from
d8c40f7
to
33c8895
Compare
elasticspoon
changed the title
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
3 times, most recently
from
4ed91bc
to
3829473
Compare
dorner
requested changes
Apr 17, 2024
elasticspoon
commented
Apr 17, 2024
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
from
96f10fc
to
bdc2a74
Compare
dorner
pushed a commit
that referenced
this pull request
Apr 25, 2024
* fix: remove more test order dependence As discussed here #4264, some tests modify global variables and expect them to be reset before each test. This makes it more difficult to move forward with removal of pre-seeding. This commit refactors certain tests to be less brittle to changes in global state and to rely less on assumptions on global state. Ex: not assuming that there are 0 records of a certain type when a test starts, etc. * fix: minor cleanup / lint
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
from
2089c64
to
b4af1bf
Compare
|
has 1 failing test but that is unrelated and fixed here: #4305 |
dorner
requested changes
Apr 26, 2024
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
from
f5a2730
to
8613397
Compare
dorner
requested changes
Apr 26, 2024
| Thread.current[:skipped_last_seeding] = true | ||
| unless self.class.metadata[:skip_seed] | ||
| if seeded_last && seed_current | ||
| define_global_variables |
There was a problem hiding this comment.
You can reorganize this a bit like so:
if seeded_last && !seed_current
DatabaseCleaner.clean_with(:truncation) # remove previously seeded data
end
if seed_current
seed_base_data_for_tests if !seeded_last # need to re-seed data
define_global_variables
end
There was a problem hiding this comment.
I was doing it the other way do be explicit about all the cases, also why I added the comment about the !seeded && !seeded case. You think this is better?
All specs tagged with seed_db will get seeded. This happens on a group level, so it will happen before the whole group. Ex: if the outer most Rspec.describe is tagged with seed_db: true the seeding will run once before the entire spec file and once after. Tracks the previous execution group. - seed or not seed base on flag on current group - truncate if the previous group seeded
Previously we were truncating if prev run had been seed and seeding if current run needed seeding. The more correct approach is: if seeded_last && seed_this => only define global vars if seeded_last && !seed_this => truncate if !seeded_last && seed_this => seed + define globals if !seed_last && !seed_this => do nothing
elasticspoon
force-pushed
the
change-item-seeding-strategy
branch
from
8613397
to
129e150
Compare
dorner
approved these changes
May 1, 2024
|
@dorner ill rebase the other existing PRs off this. You think its possible if I get the refactors in early enough to get this done before the event? |
|
Can't hurt to try! 🎉 |
|
@elasticspoon: Your PR |
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.
New change in item seeding strategy:
All specs are seeded by default except specs tagged with
skip_seed: true.Thread.current[:seeded_last]stores if the last run seeded data or not.Cost of the Change (based on how long it takes me to run the full specs)
before(:all)with lots ofseed_db: true: 12:56before(:all)with a fewskip_seed: true: 12:36