Fix incorrectly handled tests - #4432
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Standardizes test cleanup across the suite to reduce order-dependent failures by reloading settings/routes instead of manually restoring individual flags, while also removing redundant authentication cleanup.
Changes:
- Replace many per-file teardown restores with
Settings.reload!, often paired withRails.application.reload_routes!. - Remove redundant
sign_out/logoutcalls from system, integration, and controller tests. - Convert a few individual examples to
ensure-based cleanup when they mutate settings inline.
Reviewed changes
Copilot reviewed 93 out of 93 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/system/url_cookies_test.rb | Swaps logout cleanup for settings/routes reset. |
| test/system/two_factor_login_test.rb | Replaces saved flag restore with Settings.reload!. |
| test/system/secret_url_bar_test.rb | Removes setup route reload and redundant sign-out teardown. |
| test/system/push_viewing_workflows_test.rb | Adds settings/routes teardown. |
| test/system/push_cookies_test.rb | Swaps logout cleanup for settings/routes reset. |
| test/system/password_generator_test.rb | Swaps logout cleanup for settings/routes reset. |
| test/system/passphrase_protection_test.rb | Adds settings/routes teardown. |
| test/system/madmin_local_time_test.rb | Removes route reload setup and logout teardown. |
| test/system/locale_strings_test.rb | Removes route reload setup and sign-out teardown. |
| test/system/first_run_test.rb | Replaces explicit flag resets with settings/routes reload. |
| test/system/file_push_editing_test.rb | Swaps logout cleanup for settings/routes reset. |
| test/system/file_push_cookies_test.rb | Swaps logout cleanup for settings/routes reset. |
| test/system/copy_clipboard_test.rb | Removes setup route reload; uses Settings.reload! teardown. |
| test/models/url_test.rb | Replaces captured setting restore with Settings.reload!. |
| test/models/qr_test.rb | Replaces captured setting restore with Settings.reload!. |
| test/models/push_edit_test.rb | Adds Settings.reload! teardown. |
| test/models/file_push_test.rb | Replaces captured setting restore with Settings.reload!. |
| test/integration/url_push/url_push_edit_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/url/url_requested_locale_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/url/url_passphrase_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/url/url_notfound_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/url/url_json_retrieval_test.rb | Adds settings/routes teardown. |
| test/integration/url/url_json_preview_test.rb | Adds settings/routes teardown. |
| test/integration/url/url_json_passphrase_test.rb | Adds settings/routes teardown. |
| test/integration/url/url_json_deletion_test.rb | Adds settings/routes teardown. |
| test/integration/url/url_json_creation_test.rb | Adds settings/routes teardown. |
| test/integration/url/url_json_audit_test.rb | Adds settings/routes teardown. |
| test/integration/url/url_index_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/url/url_deletion_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/url/url_creation_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/url/url_anonymous_access_test.rb | Replaces explicit signup reset with settings/routes reload. |
| test/integration/require_mfa_test.rb | Replaces stored MFA flag/sign-out cleanup with Settings.reload!. |
| test/integration/qr_push/qr_push_edit_test.rb | Replaces sign-out teardown with Settings.reload!. |
| test/integration/qr/qr_requested_locale_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_passphrase_test.rb | Replaces sign-out teardown with Settings.reload!. |
| test/integration/qr/qr_notfound_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_json_retrieval_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_json_preview_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_json_passphrase_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_json_deletion_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_json_creation_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_json_audit_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_index_test.rb | Replaces sign-out teardown with Settings.reload!. |
| test/integration/qr/qr_deletion_test.rb | Adds Settings.reload! teardown; removes redundant assertions. |
| test/integration/qr/qr_creation_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/qr/qr_authenticated_test.rb | Adds Settings.reload! teardown. |
| test/integration/qr/qr_audit_test.rb | Replaces sign-out teardown with Settings.reload!. |
| test/integration/qr/qr_anonymous_access_test.rb | Replaces explicit signup reset with settings/routes reload. |
| test/integration/push_edit_remaining_values_test.rb | Adds Settings.reload! teardown. |
| test/integration/push_checkbox_attributes_test.rb | Adds settings/routes teardown. |
| test/integration/password/password_passphrase_test.rb | Moves secure-cookie cleanup into ensure with Settings.reload!. |
| test/integration/password/password_json_preview_test.rb | Adds ensure cleanup via Settings.reload!. |
| test/integration/password/password_index_test.rb | Removes redundant sign-out teardown. |
| test/integration/password/password_edit_test.rb | Removes sign-out teardown; uses Settings.reload! in blur test cleanup. |
| test/integration/password/password_deletion_test.rb | Removes redundant setting assertions. |
| test/integration/password/password_creation_test.rb | Simplifies boolean assertion style. |
| test/integration/password/password_blur_test.rb | Replaces explicit blur reset with Settings.reload!. |
| test/integration/password/password_audit_test.rb | Removes redundant sign-out teardown. |
| test/integration/password/password_anonymous_access_test.rb | Replaces class teardown with per-test ensure cleanup. |
| test/integration/locale_handling_test.rb | Replaces explicit login/flag cleanup with settings/routes reload. |
| test/integration/file_push/file_push_retrieval_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/file_push/file_push_requested_locale_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/file_push/file_push_passphrase_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/file_push/file_push_notfound_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_json_retrieval_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_json_preview_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_json_passphrase_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_json_expired_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_json_deletion_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_json_creation_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_json_audit_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_json_active_test.rb | Adds settings/routes teardown. |
| test/integration/file_push/file_push_index_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/file_push/file_push_edit_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/file_push/file_push_deletion_test.rb | Replaces sign-out teardown with settings/routes reset. |
| test/integration/file_push/file_push_creation_test.rb | Replaces sign-out cleanup with settings/routes reload; removes inline max-file restore. |
| test/integration/file_push/file_push_blur_test.rb | Replaces sign-out/blur cleanup with settings/routes reload. |
| test/integration/file_push/file_push_anonymous_access_test.rb | Replaces explicit signup reset with settings/routes reload. |
| test/integration/disable_logins_test.rb | Replaces explicit flag/sign-out resets with settings/routes reload. |
| test/integration/authentication_token_management_test.rb | Removes route reload setup and disable-logins teardown. |
| test/integration/admin_user_management_test.rb | Removes redundant sign-out cleanup from admin tests. |
| test/integration/admin_dashboard_test.rb | Removes route reload setup/teardown and redundant sign-outs. |
| test/initializers/validate_settings_test.rb | Replaces captured setting restore with Settings.reload! before initializer reload. |
| test/initializers/session_store_test.rb | Replaces captured setting restore with Settings.reload! before initializer reload. |
| test/helpers/language_helper_test.rb | Replaces manual language-code restore with Settings.reload!. |
| test/helpers/application_helper_test.rb | Replaces manual settings restore with Settings.reload!. |
| test/controllers/users/registrations_controller_test.rb | Removes disable-logins teardown. |
| test/controllers/urls_controller_test.rb | Replaces sign-out teardown and inline resets with settings/routes reload. |
| test/controllers/text_push_controller_test.rb | Replaces explicit feature resets with Settings.reload!. |
| test/controllers/qr_push_controller_test.rb | Replaces sign-out/inline resets with Settings.reload!. |
| test/controllers/first_run_controller_test.rb | Replaces explicit flag resets with settings/routes reload; removes host setup. |
| test/controllers/file_push_controller_test.rb | Replaces sign-out teardown with settings/routes reload. |
| test/controllers/api/base_controller_test.rb | Replaces explicit per-flag cleanup with Settings.reload! and trims inline ensures. |
Comments suppressed due to low confidence (4)
test/system/url_cookies_test.rb:1
- This same teardown was added to a large number of test files in this PR. Because the cleanup is now required suite-wide, keeping it duplicated per file makes future omissions likely and has already led to inconsistent cleanup in a few other updated tests. Moving the reset into the relevant base test classes would make the suite easier to keep correct.
test/controllers/first_run_controller_test.rb:1 - Removing the
default_url_options[:host]setup reintroduces the missing-host failure this test was explicitly guarding against. Any first-run path that generates an absolute URL will now depend on some earlier test having populated a host, which makes this suite order-dependent.
# frozen_string_literal: true
test/integration/authentication_token_management_test.rb:1
- This file no longer restores
Settings.disable_loginsor reloads routes between examples. Since this suite already needed explicit cleanup for login-related routing state, removing it makes the tests order-dependent again and can leave Devise routes in the wrong state for later files.
# frozen_string_literal: true
test/integration/admin_dashboard_test.rb:1
- Removing both the route reset in
setupand thedisable_logins/route cleanup inteardownmakes these admin-route assertions depend on whatever routing state a previous test left behind. Because this suite exercises route availability, it still needs an explicit route reset around each example.
# frozen_string_literal: true
馃挕 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I would like to add a cursor rule file for tests. But, |
Since this is OSS, we don't add Cursor rules here. We could add it to the Cursor dashboard for team rules... or in another repo... Can you access https://cursor.com/dashboard/team-content? Let's continue in Apnotic chat if not. |
|
BTW great improvement/cleanup PR. |
I checked different structure to be used by multiple providers(like Claude, Cursor), but every provider is suggesting different structures(like @pglombardo This PR is ready. You can merge it when you want. |
Description
Settings were changed for some tests, but they are not reverted correctly. The affected tests have been updated to restore the default settings.
Some
sign_outusages were redundant. If there is a user signed in, they are signed out automatically after finishing tests. So, these unnecessary calls are removed.config.action_mailer.default_url_optionsis added for tests.Related Issue
--
Type of Change
Checklist
rake test)-- Not necessary