Race Condition Check: spec/system/court_dates/edit_spec.rb#6733
Race Condition Check: spec/system/court_dates/edit_spec.rb#6733compwron merged 5 commits intorubyforgood:mainfrom
Conversation
Closes rubyforgood#6329 - fixes one factory creation (it was creating admin twice) - checks for explicit values in the page - avoids checking the database for changes, and checks for what the user sees when interacting with the page
stefannibrasil
force-pushed
the
sb-court-dates-system-spec
branch
from
d46d872 to
fb59a36
Compare
stefannibrasil
requested review from
FireLemons,
compwron and
elasticspoon
as code owners
stefannibrasil
changed the title
spec/system/court_dates/edit_spec.rb
Outdated
| click_on casa_case.case_number | ||
| click_on "CINA-08-1001" |
There was a problem hiding this comment.
Can you DRY all these hard coded values? If multiple different but equal values are meant to refer to the same value, they should be referenced using the same variable.
There was a problem hiding this comment.
I can do that, sure. I agree with that for code outside of tests but not so much for tests. I am on the side that, especially for system tests, having explicit values like this is best for understanding the business logic of the app and debugging, when needed. By having explicit values, one does not need to evaluate the variable and jump straight to what the test is covering.
For example, checking for court_date.date.strftime("%B %-d, %Y") requires the developer to go and evaluate that when debugging a test. It's small but having the explicit date saves time, and keeps the test close to what the app renders.
Reverted it to your request in 39ee510
|
@FireLemons the docker error is unrelated. Has this happened before? Thank you for your review! |
There was a problem hiding this comment.
Pull request overview
This PR fixes race conditions in the court dates edit system spec by replacing database state checks with page content verifications. The changes ensure tests wait for browser interactions to complete before making assertions, preventing flaky test failures on slower GitHub servers.
Changes:
- Fixed factory creation to use supervisor instead of creating admin twice
- Replaced database count checks (
expect(CourtDate.count)) with page content assertions - Improved test descriptions to use "allows/does not allow" naming convention
- Modernized alert handling by replacing
page.driver.browser.switch_to.alert.acceptwithaccept_alert
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| accept_alert "Are you sure?" do | ||
| page.find("a", text: "Delete Future Court Date").click | ||
| end | ||
| expect(page).to have_content "Court date was successfully deleted" |
There was a problem hiding this comment.
The expected success message is missing a period at the end. The controller at app/controllers/court_dates_controller.rb:58 returns "Court date was successfully deleted." with a period, but this test expects it without one. The supervisor test at line 105 has the correct expectation with the period. This inconsistency should be fixed to match the actual message returned by the controller.
| expect(page).to have_content "Court date was successfully deleted" | |
| expect(page).to have_content "Court date was successfully deleted." |
Closes #6329