Reduce duplication of fixture SHAs in tests #15610
Open
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
Having the SHA in the path makes it harder to script updates to the fixtures and means that the knowledge of the current SHA is duplicated between `countries.json` and the path. We do want to track the SHA of the fixtures, but this information is already in git in the `countries.json` fixture and in the VCR cassettes, so we don't need to also be tracking it in the directory names in the fixtures and in the tests themselves.
This removes the SHA argument from the stub_popolo method. We don't want to couple ourselves to the SHA in too many places, as it makes updating the test fixtures much harder than it needs to be.
This makes the tests use stub_popolo consistently, whereas before we were using stub_popolo in some places and stub_everypolitician_data_request in others.
We no longer include the SHA in the tests, so this rake task needs to lookup the current SHA for the requested legislature using the local `countries.json` file. Then it can use this information to grab the correct version of the requested file.
|
Have popped another commit on the end of this to fix the rake which downloads fixtures. So I think this is actually ready for review now. |
|
This is needed because the tests are currently using the |
|
@chrismytton I suspect it's probably best if you could resolve all the conflicts before this gets reviewed… |
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 does this do?
This reduces the number of places that we're storing the SHA of the fixtures in the tests.
Why was this needed?
Previously we were storing the SHA in the following places:
In the VCR cassettesMy mistake, we're not using VCR on this project!countries.jsonfixturestub_popolo('faa324', 'UK/Commons')This meant that updating the fixtures to a newer version (which we want to do as part of adding cabinet memberships #24, and probably again when we finalise where that data is going to live), was quite a complicated and hard to script process.
So this change removes the SHA from the fixture directory name and the tests themselves, which should be enough to make updating the fixtures relatively painless.
Relevant Issue(s)
Fixes #15607
Unblocks #15606
Notes to Reviewer
As part of this change I've also switched some places that were still using
stub_everypolitician_data_requestto use the newerstub_popolomethod. I've done this in a separate commit so that can be split off into a separate pull request if preferred.