Improve factories#2922
Open
thomasiles wants to merge 8 commits into
Open
Conversation
Remove the hardcoded organisation id, which I *think* but haven't confirmed is the reason for the test being flaky.
The Form factory trait, :with_pages builds a new form for each page which isn't used. To see the issue, open the rails console compare the number of forms before and after using the factory and trait. ```ruby Form.count f=FactoryBot.create(:form, :with_pages) Form.count ``` You will see six forms created when only one should be made. Instead we build the pages and set the form to nil. Enable logging SQL statements in the console to see how much extra work is involved without this change. ```ruby ActiveRecord::Base.logger = Logger.new(STDOUT) ActiveRecord::LogSubscriber.attach_to :active_record ```
Remove hardcoded id which can cause deadlocks in the database when two tests run concurrently. We also updates the membership factory to build associations and share one organisation instance between user, group, and added_by. Without this, build(:membership) would compare two in-memory Organisation objects with nil ids (now that the hardcoded id=1 is gone) and fail the 'User and group must have the same organisation' validation. Sharing a user means build(:membership) touches no tables and create(:membership) no longer races two Organisation inserts against the unique slug index.
The organisation factory dedups every default-built organisation onto a single canonical test-org row, so specs could create users (via auth helpers) and groups (via the bare :group factory) and get the same organisation by accident. We want to remove the dedup part of the Organisation factory, so that we can build them without touching the database. This commit changes specs to make the sharing explicit.
Our specs used to rely on the fact that orgs with the same slug would be identical. Now that we've removed that pattern from the specs and made it explicit, we can remove code from the factory that was ensuring that all orgs created with the same slug were identical. This caused the DB to be used, even when using build as the select would always be called. Instead, we give organisations a unique sequence-based slug.
Using create in a factory means even factories built with 'build' or 'build_stubbed' will hit the database.
User's belong to Organisations except when they first sign up. We have validation that ensures that once a user has an organisation they move to a new organisation but can't be without one. The existing validation accessed the database column directly. This meant if you build a User and add an Organisation to it, it will be invalid. Instead, we add a check to only run the validation if there is no Organisation yet.
The MOU signature factory was using create to make a user, which meant using build would still use the database. This caused us to use create in Org, too. We needed to use create to make the user valid because of the validation on User. Now we've fixed that, we can use build instead. We also change the org factory. Because we are now using build instead of create for users, the view specs failed when using a path helper that relied on the user's id. We fix that by passing build_stubbed(:user) in the specs so there is an id.
|
🎉 A review copy of this PR has been deployed! You can reach it at: https://pr-2922.admin.review.forms.service.gov.uk/ It may take 5 minutes or so for the application to be fully deployed and working. If it still isn't ready For the sign in details and more information, see the review apps wiki page. |
| context "and the user's organisation is not the same as given" do | ||
| it "returns false" do | ||
| user = create(:organisation_admin_user) | ||
| other_org = build(:organisation, id: 2) |
Contributor
There was a problem hiding this comment.
We've got the hardcoded org id, which could clash now?
| let(:user_with_access_admin) { create(:user, email: "admin_user@example.com", organisation: signed_mou_org) } | ||
| let(:user_without_access) { create(:user, email: "inactive_user@example.com", has_access: false, organisation: signed_mou_org) } | ||
| let(:user_access_and_admin_with_mou) { create(:user, email: "admin_mou_user@example.com", organisation: signed_mou_org) } | ||
| let(:non_crown_org) { create(:organisation, id: 2, slug: "non-crown-org") } |
Contributor
There was a problem hiding this comment.
Drop the hard coded org id here? as will now collide with signed_mou_org?
|
|
||
| def requires_organisation? | ||
| organisation_id_was.present? || role_changed?(to: :organisation_admin) | ||
| organisation.blank? && (organisation_id_was.present? || role_changed?(to: :organisation_admin)) |
Contributor
There was a problem hiding this comment.
this is going to add extra db query every time, this function is called - is that okay?
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Improvements to our test Factories
Trello card: https://trello.com/c/L37Bdby6/260-improve-the-tests-in-forms-admin
Our FactoryBot tests have become a bit tangled. I'm trying to uncouple them a bit and stop using the database when it's not needed and stop hardcoding database ids, which, I think, causes reliability problems when tests are run in certain orders or at the same time.
This PR tackles some of the issues which stop us using build, which doesn't use the database instead of create. It doesn't switch any tests yet, it's just preparing the groundwork.
One of the biggest issues is how we treat organisations. I've tried to keep the commits small but one commit makes small changes to a lot of specs which relied on orgs being the same.
You can use the rails console to test the factories with
Factorybot.createorFactoryBot.buildenabling SQL logging is also useful:Things to consider when reviewing