Patch #42422
closed
Use Capybara's assert_current_path in "log_user" steps to wait for page in ApplicationSystemTestCase
0%
Description
- Problem
The login step in the system test sometimes fails because the expected page load isn’t complete when the assertion is executed as follows.
$ cd ../redmine && RAILS_ENV=test bin/rails redmine:plugins:test NAME=full_text_search # Running: ...............Capybara starting Puma... * Version 6.6.0 , codename: Return to Forever * Min threads: 0, max threads: 4 * Listening on http://127.0.0.1:43813 .[Screenshot Image]: /home/runner/work/redmine_full_text_search/redmine/tmp/screenshots/failures_test_keep_search_target.png F Failure: FullTextSearch::SearchTest#test_keep_search_target [test/application_system_test_case.rb:79]: --- expected +++ actual @@ -1 +1 @@ -"/my/page" +"/login" rails test plugins/full_text_search/test/system/full_text_search/search_test.rb:58 ............SSS.......................................................................... Finished in 124.323414s, 0.8526 runs/s, 1.0778 assertions/s. 106 runs, 134 assertions, 1 failures, 0 errors, 3 skips
ref: https://github.com/clear-code/redmine_full_text_search/actions/runs/13889360454/job/38858640197
- Cause
The test was using assert_equal to verify the current path immediately after form submission.
Since assert_equal does not wait for the page to load fully, it leads to intermittent failures in environments where page rendering is slower.
- Solution
Replace assert_equal with assert_current_path in the login steps.
This change leverages Capybara’s built-in waiting mechanism, ensuring that the test only proceeds once the browser has navigated to the expected page.
ref: https://www.rubydoc.info/gems/capybara/Capybara%2FSessionMatchers:assert_current_path
Files
Updated by Takuya Kodama 2 months ago
I'm so sorry I should use `Code cleanup/refactoring` category this time.
Updated by Takuya Kodama 2 months ago
- File 0001-v2-Use-Capybara-s-assert_current_path-in-login-steps-to.patch 0001-v2-Use-Capybara-s-assert_current_path-in-login-steps-to.patch added
Previous patch couldn't handle the path with query parameters.
Adding the `:ignore_query => true` option to `assert_current_path` allows the test assertions to ignore any query parameters present in the current URL, providing greater flexibility and robustness when query parameters are appended dynamically.
Updated by Marius BĂLTEANU 2 months ago
- Description updated (diff)
- Category changed from My page to Code cleanup/refactoring
Updated by Marius BĂLTEANU 2 months ago
- Subject changed from Use Capybara's assert_current_path in `log_user` steps to wait for page in ApplicationSystemTestCase to Use Capybara's assert_current_path in "log_user" steps to wait for page in ApplicationSystemTestCase
- Description updated (diff)
- Assignee set to Marius BĂLTEANU
- Target version set to Candidate for next minor release
Updated by Katsuya HIDAKA about 2 months ago
The failure of the log_user method test may be related to the following issue reported in Capybara:
Chrome 134 breaks Selenium tests - intermittent failures with visit
In fact, it does not reproduce in my environment (Chrome 131, selenium-driver 4.27.0).
Even so, the patch in #note-2 is a reasonable improvement and it would be good to apply it. I have confirmed that applying the patch in #note-2 resolves the failure of the log_user method test.
Also, the following test failure seems to be affected by the issue reported in Capybara:
Failure: IssuesSystemTest#test_create_issue_with_attachment [test/test_helper.rb:231]: "Issue.count" didn't change by 1, but by 0. Expected: 15 Actual: 14 bin/rails test test/system/issues_test.rb:134
Updated by Takuya Kodama about 2 months ago
Thank you for giving me the above information and confirming my patch.
Updated by Marius BĂLTEANU 30 days ago
- Target version changed from Candidate for next minor release to 5.1.9
Updated by Katsuya HIDAKA 11 days ago
Updated by Takuya Kodama 8 days ago
Do we have any blockers to merge these patches? If so, could you tell me that?
Updated by Marius BĂLTEANU 7 days ago
- Status changed from New to Resolved
Committed, thanks!
Takuya Kodama wrote in #note-9:
Do we have any blockers to merge these patches? If so, could you tell me that?
No, just lack of time.
Updated by Katsuya HIDAKA 7 days ago
Thank you for committing the change.
However, r23761 removed the visit '/my/page' line from the log_user method, which is causing all system tests to fail. It needs to be fixed as follows:
diff --git a/test/application_system_test_case.rb b/test/application_system_test_case.rb
index 040667c39..726e7af73 100644
--- a/test/application_system_test_case.rb
+++ b/test/application_system_test_case.rb
@@ -72,6 +72,7 @@ class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
# Should not depend on locale since Redmine displays login page
# using default browser locale which depend on system locale for "real" browsers drivers
def log_user(login, password)
+ visit '/my/page'
assert_current_path '/login', :ignore_query => true
assert_equal '/login', current_path
within('#login-form form') do
Ive confirmed that all system tests pass after applying this fix:
https://github.com/hidakatsuya/redmine/actions/runs/15033489238/job/42250775968
Updated by Marius BĂLTEANU 7 days ago
Katsuya HIDAKA wrote in #note-12:
Thank you for committing the change.
However, r23761 removed the
visit '/my/page'line from thelog_usermethod, which is causing all system tests to fail. It needs to be fixed as follows:[...]
Ive confirmed that all system tests pass after applying this fix:
https://github.com/hidakatsuya/redmine/actions/runs/15033489238/job/42250775968
Sorry for that, I've fixed it now, thanks!