run after_teardown in system specs after around (like in other specs)

[timdiggins]

This will only work as expected (print out screenshot location after failed tests)
in >= rails 6.0 but 5.2 is no longer supported in main.

This may possibly change behaviour, but can be included in major
revision?

This should enable easier integration of other libraries such as
capybara-screenshot which require predictable (and late) running of
Capybara.reset_sessions!

This is an illustration (or possible candidate to fix) #2595

[pirj]

Might also be related to #2526, but to my understanding this change won't fix it.

[JonRowe]

Can you write up a test for this that fails without the change?

[timdiggins]

Might also be related to #2526, but to my understanding this change won't fix it.

I think the reason (some) after hooks have no access to the page is because the reset_sessions! has already occurred. If this is the case then this would fix it. However if you are loading rspec-rails first (before requiring any other gems / adding otheer around hooks), I think that the reset_sessions should occur last (because after always load before, and the reset_sessions! after is created first when rspec-rails is first required). But if you do any append_after those would happen after reset_sessions!

[timdiggins]

Can you write up a test for this that fails without the change?

Before I write this, can I try writing some pseudo code and see if this makes sense:

Given: A custom (system spec only) notifier/formatter that relies on presence of capybara session
When: you run this example system spec in the presence of this notifier
Then: the custom notifier can't access the capybara session (and then either fails to print, or could raise an exception)

If that's the kind of thing, then I could look at notifier/formatter specs for a template? @JonRowe

[JonRowe]

I'd write a integration test that shows that the hook methods are called in the order you want to change this to, you could add a fake 3rd party module to show that is executed correctly in the sequence, but this change distills down to the order of which our hooks interact with the "test unit" hooks.

[timdiggins]

@JonRowe Thanks for the suggestion for the approach for writing the test -- really helped.

I started by writing a test that wanted the Capybara.reset_sessions! call (in the after_teardown hook) to happen after all the after hooks AND after the notification for "example_failed" ran. But I'm not sure if this is possible or even actually desirable.

Here's my thinking:

1. Capybara.reset_sessions! needs to occur before the rollback of the transaction (this is so that any lurking ajax calls for example are stopped before the transaction rolls back - otherwise you can get these happening at the beginning of the next spec surprising)
2. We do want this as late as possible - no after hook (or ideally any around hook) should get this
3. However a formatter handling an example_failed notification shouldn't necessarily expect capybara to still have valid sessions (ie. the reset_sessions! to have not yet happened). This is capybara-screenshot's current approach.

Therefore I still want to change how the ordering of the callbacks (for 2 above).

I think here it would be better if capybara-screenshot (for system specs) monkey-patched the rails take_screenshot (which it is in effect improving on) and either let rspec-rails capture output, or if it's still using a formatter.

So I'll update the PR to have implement 2 above and see if it fixed the issue with capybara-screenshot. I'll get to it when I can... In the meantime (or afterwards) feedback welcome

UPDATE: My bad - I based my first version of this on faulty memory. Capybara-screenshot does use an after hook so maybe changing the order will fix its integration with system specs.

[timdiggins]

@JonRowe I've updated my PR as discussed with a test and a fix. It seems to fix some the integration with capybara-screenshot

[timdiggins]

FYI I've also tested manually that:

• given a vanilla rails 6.1 app (with this branch of rspec-rails) when theres a failing system test, it takes a screenshot (via rails mechanism) and outputs the path
• given a vanilla rails 6.1 app (with this branch of rspec-rails, and the latest Capybara-screenshot), it takes a screenshot via capybara-screenshot and outputs the html and png paths.

NB with capybara-screenshot, both the capybara-screenshot and the rails mechanisms run and output their paths. I think this can be resolved (or lived with) either in capybara-screenshot, or the specific rails-app repo.

One more note. It's not easily possible to backport this fix into supporting rails 5.2 while supporting the rails screenshot because this happens in the same rails callback.

[dgm]

So .. has the screenshot capability really been broken since Rails 5.1 ?

[timdiggins]

So .. has the screenshot capability really been broken since Rails 5.1 ?

In system specs, capybara-screenshot has not been printing out screenshot locations on failure (though saving the files). Feature specs working fine.