Save screenshot path on system test failure #48863

matteeyah · 2023-08-01T13:30:42Z

Motivation / Background

Lots of CI tools support showing a failure screenshot somewhere in their UI (e.g. GitLab). I want to persist the screenshot path so these tools can pick up the screenshot and visually display it.

Detail

This Pull Request changes ActionDispatch::SystemTesting::TestHelpers::ScreenshotHelper to save the screenshot path in the test metadata on failure.

Additional information

Bumping minitest to 5.19.0 is required because the metadata changes were added in then.

https://github.com/minitest/minitest/blob/master/History.rdoc#5190--2023-07-26-

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

eileencodes · 2023-08-01T16:22:22Z

actionpack/lib/action_dispatch/system_testing/test_helpers/screenshot_helper.rb

+          return unless failed? && supports_screenshot? && Capybara::Session.instance_created?
+
+          take_screenshot
+          metadata[:failure_screenshot_path] = absolute_image_path


Bumping minitest in the gemfile only does it for the Rails CI builds but not applications. We locked an older Ruby to an older minitest becasue of the kwargs changes. You can see this is causing the tests to fail for that one here: https://buildkite.com/rails/rails/builds/98449#0189b14e-eeaf-4d2d-9b0f-5382c140fe23/1062-1226

We can't set this or test it unless metadata is defined. We also don't want to force applications to upgrade to 5.19 for this feature.

I'm a big fan of your work @eileencodes! I feel like talking to a real life action heroine. 😊

Back to reality - I'm a new contributor so I'll rehash your comment to make sure I understood it properly. You're saying we need to check if metadata is defined here before doing anything with it? 🤔 This will then allow downstream apps to use either version of minitest.

Is that preferable to backporting the functionality to older versions of minitest? Not sure if the the minitest maintainer is open to the idea, but I could try asking.

I added a check for the existence of metadata.

To make the tests work, I skipped the test dynamically if metadata is not defined on Minitest::Runnable. This leaks a little bit of implementation into the test, but I wasn't sure how else I could make the test work for older versions of minitest.

I also contemplated adding an additional test case that checks how the helper behaves in a scenario when it's running with an older version of minitest which doesn't have metadata defined. i.e. to make sure that it doesn't raise an error. But in order to do that, I'd need to undefine metadata then redefine it after running the test. I opted against this, because I deemed the risk of the test leaking bigger than the risk of the error that it's trying to prevent.

casperisfine · 2023-08-03T08:03:17Z

actionpack/test/dispatch/system_testing/screenshot_helper_test.rb

@@ -152,6 +152,24 @@ def setup
    assert_match %r|url=artifact://.+?tmp/screenshots/1_x\.png|, display_image_actual
  end

+  test "take_failed_screenshot persists the image path in the test metadata" do
+    skip "Older versions of Minitest don't support test metadata." unless Minitest::Runnable.instance_methods(false).include?(:metadata)


Suggested change

skip "Older versions of Minitest don't support test metadata." unless Minitest::Runnable.instance_methods(false).include?(:metadata)

skip "Older versions of Minitest don't support test metadata." unless Minitest::Runnable.method_defined?(:metadata)

I changed it with your suggestion.

casperisfine · 2023-08-03T08:03:50Z

actionpack/lib/action_dispatch/system_testing/test_helpers/screenshot_helper.rb

+          return unless failed? && supports_screenshot? && Capybara::Session.instance_created?
+
+          take_screenshot
+          metadata[:failure_screenshot_path] = absolute_image_path if defined?(metadata)


Suggested change

metadata[:failure_screenshot_path] = absolute_image_path if defined?(metadata)

metadata[:failure_screenshot_path] = absolute_image_path if Minitest::Runnable.method_defined?(:metadata)

Otherwise this code could be confused by a user defined metadata method in the test class.

It makes sense being more explicit when checking for this. I applied your suggestion. Thanks.

This now fails when minitest isn't the test runner, for example when rspec is the runner. Is there an additional condition that can be added to detect that minitest is the runner and not just loaded?

@albus522 Does the test fail or is there just no additional metadata? Could you open an issue with a repro? 🙇

@zzak @matteeyah I reproduced the error reported by @albus522 with a video.
Let me know how I can help solving this. Thanks 🙇

rspec.rails7.issue.1080.mov

This was fixed today by @JonRowe in rspec/rspec-rails#2704 🎉

Thanks for the thorough explanation! Glad it was fixed downstream. 🙏

casperisfine · 2023-08-03T12:40:18Z

Please squash your commits, after that it's good for me.

byroot · 2023-08-03T14:35:36Z

Thank you!

davidwessman · 2023-08-05T06:41:55Z

Exactly what I wanted in #44624 😍

rails-bot bot added the actionpack label Aug 1, 2023

matteeyah force-pushed the main branch from 3f5e8b3 to 20537ce Compare August 1, 2023 13:34

eileencodes requested changes Aug 1, 2023

View reviewed changes

matteeyah force-pushed the main branch 2 times, most recently from f147e5e to 3415140 Compare August 2, 2023 12:56

matteeyah requested a review from eileencodes August 2, 2023 13:08

casperisfine reviewed Aug 3, 2023

View reviewed changes

matteeyah force-pushed the main branch from 3415140 to cd875f3 Compare August 3, 2023 12:10

Save failure screenshot path in test metadata

a962fc3

matteeyah force-pushed the main branch from cd875f3 to a962fc3 Compare August 3, 2023 12:43

byroot merged commit ee3117b into rails:main Aug 3, 2023
9 checks passed

matteeyah mentioned this pull request Aug 4, 2023

Output failure screenshot when available minitest-reporters/minitest-reporters#346

Merged

matteeyah mentioned this pull request Aug 5, 2023

Use relative path for screenshot metadata #48893

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save screenshot path on system test failure #48863

Save screenshot path on system test failure #48863

matteeyah commented Aug 1, 2023 •

edited

eileencodes Aug 1, 2023

matteeyah Aug 2, 2023 •

edited

matteeyah Aug 2, 2023

casperisfine Aug 3, 2023

matteeyah Aug 3, 2023 •

edited

casperisfine Aug 3, 2023

matteeyah Aug 3, 2023

albus522 Oct 6, 2023

zzak Oct 7, 2023

claudiob Nov 14, 2023

claudiob Nov 15, 2023

zzak Nov 25, 2023

casperisfine commented Aug 3, 2023

byroot commented Aug 3, 2023

davidwessman commented Aug 5, 2023

	skip "Older versions of Minitest don't support test metadata." unless Minitest::Runnable.instance_methods(false).include?(:metadata)
	skip "Older versions of Minitest don't support test metadata." unless Minitest::Runnable.method_defined?(:metadata)

	metadata[:failure_screenshot_path] = absolute_image_path if defined?(metadata)
	metadata[:failure_screenshot_path] = absolute_image_path if Minitest::Runnable.method_defined?(:metadata)

Save screenshot path on system test failure #48863

Save screenshot path on system test failure #48863

Conversation

matteeyah commented Aug 1, 2023 • edited

Motivation / Background

Detail

Additional information

Checklist

Choose a reason for hiding this comment

matteeyah Aug 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteeyah Aug 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

casperisfine commented Aug 3, 2023

byroot commented Aug 3, 2023

davidwessman commented Aug 5, 2023

matteeyah commented Aug 1, 2023 •

edited

matteeyah Aug 2, 2023 •

edited

matteeyah Aug 3, 2023 •

edited