If take_failed_screenshot fails, the rest of the teardown hooks do not run, breaking transactions

[markiz]

What Ruby, Rails and RSpec versions are you using?

Ruby version: 2.6.3
Rails version: 6.0.3.4

RSpec 3.9

• rspec-core 3.9.3
• rspec-expectations 3.9.3
• rspec-mocks 3.9.1
• rspec-rails 4.0.1
• rspec-support 3.9.4

Observed behaviour

Sometimes take_failed_screenshot would fail, because driver raised an exception. For us, Ferrum::DeadBrowserError was the most common issue. When this happens, all the consequent teardown hooks do not run, including the transactional fixtures teardown (which would normally roll back the wrapper transaction). That leaves the suite with a dirty db state.

Expected behaviour

I'm not sure. Probably, after_teardown hooks should run even if before_teardown hooks failed?

Can you provide an example app?

Alas, no, but here's an example patch that helped us with this issue:

module ScreenshotHelperPatch
  def take_failed_screenshot(*)
    super
  rescue => e # Silently ignore errors
  end
end

# And in the rails_helper
config.include ScreenshotHelperPatch, type: :system

I guess you can emulate this situation by raising from one of the before_teardown callbacks

[klyonrad]

Hmm the code from rails should actually rescue/ensure the calls of take_failed_screenshot. @markiz Could you maybe try out reproducing the issue with a non rspec test in your project and report back?

Nevertheless I have a suspicion. Rails tries to call super of before_teardown when take_failed_screenshot causes an exception. But rspec-rails defines that method as an empty method. That might override whatever rails is trying to calls in the ensure block (I don't understand it). Understanding the ancestor tree that rspec-rails file is too complex for me so I might be totally wrong.

[markiz]

@klyonrad it does not rescue, that's the issue. It bubbles the exception up the chain.

[JonRowe]

Yes but neither does Rails, which arguably owns this behaviour, this must happen in mini test too no?

[markiz]

Yes but neither does Rails, which arguably owns this behaviour, this must happen in mini test too no?

Unfortunately, I don't know.

My expectation would be that a failure in a teardown hook makes a spec fail but does not prevent other teardown hooks from being called.

I believe it became a somewhat big issue in rails 6 because they moved this screenshot call from after_teardown to before_teardown, and that makes transactions not roll back, resulting in cascade failures of all subsequent specs. Normally we would just be able to rerun just the failing spec a couple of times and not have a failure in one spec affect all the others.