Merge pull request #36047 from rmacklin/take-screenshot-sooner

Make system tests take failed screenshots in `before_teardown` hook

3 files changed, 42 insertions, 5 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 4109ae7006..4502c6a2f9 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,12 @@
+*   Make system tests take a failed screenshot in a `before_teardown` hook
+    rather than an `after_teardown` hook.
+
+    This helps minimize the time gap between when an assertion fails and when
+    the screenshot is taken (reducing the time in which the page could have
+    been dynamically updated after the assertion failed).
+
+    *Richard Macklin*
+
 *   Introduce `ActionDispatch::ActionableExceptions`.
 
     The `ActionDispatch::ActionableExceptions` middleware dispatches actions
diff --git a/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb b/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb
index 600e9c733b..7080dbe022 100644
--- a/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb
+++ b/actionpack/lib/action_dispatch/system_testing/test_helpers/setup_and_teardown.rb
@@ -16,12 +16,14 @@ module ActionDispatch
           super
         end
 
+        def before_teardown
+          take_failed_screenshot
+        ensure
+          super
+        end
+
         def after_teardown
-          begin
-            take_failed_screenshot
-          ensure
-            Capybara.reset_sessions!
-          end
+          Capybara.reset_sessions!
         ensure
           super
         end
diff --git a/railties/test/application/test_runner_test.rb b/railties/test/application/test_runner_test.rb
index 0bdd2b314d..1ab45abcd0 100644
--- a/railties/test/application/test_runner_test.rb
+++ b/railties/test/application/test_runner_test.rb
@@ -794,6 +794,32 @@ module ApplicationTests
       assert_match "Capybara.reset_sessions! called", output
     end
 
+    def test_failed_system_test_screenshot_should_be_taken_before_other_teardown
+      app_file "test/system/failed_system_test_screenshot_should_be_taken_before_other_teardown_test.rb", <<~RUBY
+        require "application_system_test_case"
+        require "selenium/webdriver"
+
+        class FailedSystemTestScreenshotShouldBeTakenBeforeOtherTeardownTest < ApplicationSystemTestCase
+          ActionDispatch::SystemTestCase.class_eval do
+            def take_failed_screenshot
+              puts "take_failed_screenshot called"
+              super
+            end
+          end
+
+          def teardown
+            puts "test teardown called"
+            super
+          end
+
+          test "dummy" do
+          end
+        end
+      RUBY
+      output = run_test_command("test/system/failed_system_test_screenshot_should_be_taken_before_other_teardown_test.rb")
+      assert_match(/take_failed_screenshot called\n.*test teardown called/, output)
+    end
+
     def test_system_tests_are_not_run_with_the_default_test_command
       app_file "test/system/dummy_test.rb", <<-RUBY
         require "application_system_test_case"