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 3 months ago
I'm so sorry I should use `Code cleanup/refactoring` category this time.
Updated by Takuya Kodama 3 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 3 months ago
- Description updated (diff)
- Category changed from My page to Code cleanup/refactoring
Updated by Marius BĂLTEANU 3 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 3 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 3 months ago
Thank you for giving me the above information and confirming my patch.
Updated by Marius BĂLTEANU about 2 months ago
- Target version changed from Candidate for next minor release to 5.1.9
Updated by Katsuya HIDAKA about 1 month ago
Updated by Takuya Kodama 29 days ago
Do we have any blockers to merge these patches? If so, could you tell me that?
Updated by Marius BĂLTEANU 29 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 28 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 28 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!
Updated by Takuya Kodama 24 days ago
Thank you so much for handling this issue!