From ef12ccfd8bc42d88611dea1190988214836b951c Mon Sep 17 00:00:00 2001
From: Richard Macklin <richard.github@nrm.com>
Date: Sat, 20 Apr 2019 19:09:50 -0700
Subject: [PATCH] Make system tests take failed screenshots in
 `before_teardown` hook

Previously we were calling the `take_failed_screenshot` method in an
`after_teardown` hook. However, this means that other teardown hooks
have to be executed before we take the screenshot. Since there can be
dynamic updates to the page after the assertion fails and before we
take a screenshot, it seems desirable to minimize that gap as much as
possible. Taking the screenshot in a `before_teardown` rather than an
`after_teardown` helps with that, and has a side benefit of allowing
us to remove the nested `ensure` commented on here:
https://github.com/rails/rails/pull/34411#discussion_r232819478
---
 actionpack/CHANGELOG.md                       |  9 +++++++
 .../test_helpers/setup_and_teardown.rb        | 12 +++++----
 railties/test/application/test_runner_test.rb | 26 +++++++++++++++++++
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 4109ae70061ba..4502c6a2f9f17 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 600e9c733b1cd..7080dbe0225ab 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 @@ def before_setup
           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 0bdd2b314d704..1ab45abcd025f 100644
--- a/railties/test/application/test_runner_test.rb
+++ b/railties/test/application/test_runner_test.rb
@@ -794,6 +794,32 @@ def reset_sessions!
       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"