Fix missing failed examples if rspec hooks fail

[zhming0]

[Skipping my journey of discovering this because this is a public repo....]

Rspec's around hook has some undocumented behavoir, look at this example:

RSpec.configure do |config|
  config.around(:each) do |example|
    puts "Outer around hook start"
    begin
      example.run
    ensure
      puts "Outer around hook end"
    end
  end
end

RSpec.configure do |config|
  config.around(:each) do |example|
    puts "Inner around hook start"
    raise "Error in inner hook"
    example.run
    puts "Inner around hook end"
  end
end

RSpec.describe "Example" do
  it "does something" do
    expect(true).to be true
  end
end

If we don't use ensure then neither "Inner around hook end" and "Outer around hook end" gets executed.

---

In another word, prior to this fix, any Rspec test fails that's caused by around hook failure will be missed.

[wooly]

@zhming0 this definitely looks a bit suspicious (the issue, rather than the solution)! what's the output for the Example test? What's the output if you do fail inside the test instead of the true == true expectation?

[zhming0]

@wooly in the case when the test itself broke, around hook gets fully executed.

The output in my example:

Outer around hook start
Inner around hook start
Outer around hook end

(otherwise we would have a completely broken collector 😅 )

[niceking]

I think I follow your change but I'm missing a bit of context still.

I follow the code change - I think what you're saying is that we need to add a begin ... ensure block around running the example, and then ensuring that the result is persisted, so that any errors that are raised as part of the example running don't bubble up and cause the error result to not be stored

I don't really follow what you're trying to illustrate with the example in the readme?

[niceking]

I found your linear ticket that I won't link here 😛

I agree that this change works and fixes the issue of test failures not being ingested.

I don't think it has anything to do with the configuration of around hooks in places that aren't this repository? I'm just a little confused as to what you're trying to explain with the PR description, it may just be a case of editing the description to match the change though 🤷‍♀️

[niceking]

Hahaha ok re-reading the thread for the THIRD time and maybe making sense of it now 😛

Are you saying that this change fixes the ingestion of failed tests, where the test is failing due to an around hook (not configured by the collector) throwing an exception?

This still seems like the correct change to make but I'd like to just get a bit more context about testing.

My understanding of rspec before, after, and around hooks is that you can have heaps of them and they get executed in reverse order in which they are loaded. I'm wondering if the issue you're fixing here works for execeptions thrown in around blocks loaded before the collector, or after the collector, or both?

[zhming0]

@niceking sorry for the confusion.

Are you saying that this change fixes the ingestion of failed tests, where the test is failing due to an around hook (not configured by the collector) throwing an exception?

Correct.

I will try my best explaining why this is the fix, but happy to do a zoom catchup if it helps.

How this plugin works is through around hook. Target project call our hook setup code.

We expect when a test fail, no matter for what reason, our around hook's after code will be executed.

However, without the fix, other around hook's failure will stop our around hook's after code to be executed. This is explained in the example in PR description.

I'm wondering if the issue you're fixing here works for execeptions thrown in around blocks loaded before the collector, or after the collector, or both?

This is a good question. This works for around hooks declared after the collector's around hooks setup. We expect this plugin to be loaded earlier than most around hooks so this is okay.

[nprizal]

I've looked into this issue last year and wrote a Basecamp post about it, but we decided to put it as low priority at that time. This solution makes sense to me, and I think it's similar to what I had in mind then. Good to see it being fixed 😊

[niceking]

Omg @nprizal I completely forgot about this post!! Good rememebering :D