Indent all extra failure lines correctly in system tests

[agrobbin]

This is my first contribution to rspec-rails, so if I've missed anything, or done anything wrong, please let me know!

If the output from Rails' system test teardown is multiple lines, we should try and render all of the lines with proper indentation.

This can happen on rails/rails@master now that failure screenshots can include the page HTML (rails/rails@36545), and can actually happen with v6.0.2.2 if the method_name ends up being a little too long.

[JonRowe]

👋 Can you add some tests and provide a sample output before/after? Cheers!

[agrobbin]

@JonRowe here's some sample output:

Before:

Failures:

  1) Admin deals @mentioning contacts in activity stream trying to mention a non-admin       Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }

     Failure/Error: Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }
       expected to find css ".tribute-cntainer li" but there were no matches

     [Screenshot]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}
_152.png
[Screenshot HTML]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}
_152.html

After:

Failures:

  1) Admin deals @mentioning contacts in activity stream trying to mention a non-admin       Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }

     Failure/Error: Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }
       expected to find css ".tribute-cntainer li" but there were no matches
     
     [Screenshot]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}

     _314.png

     [Screenshot HTML]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}

     _314.html

I'm honestly not entirely sure why there are newlines being added before the _314.{png,html}, though the only thing I can think of is something to do with this. Either way though, that seems to be not an issue with RSpec (or rspec-rails), but with Rails itself.

As for tests, I initially started to write some tests for this, but ran into a bit of a roadblock when trying to figure out how best to write a test for something like this. This is about where I got stuck:

describe '#after' do
  it 'sets the :extra_failure_lines metadata to an array of STDOUT lines' do
    group = RSpec::Core::ExampleGroup.describe do
      include SystemExampleGroup

      def original_after_teardown
        puts 'foo'
        puts 'bar'
      end
    end
    example = group.new
    group.hooks.run(:after, :example, example)

    expect(example.metadata[:extra_failure_lines]).to eq(['foo', 'bar'])
  end
end

Do you have some guidance on that?

[JonRowe]

Hello!

Having looked at your output I'd say the first is correct, but Rails is wrapping across to a second line?

e.g. I'd expect

Failures:

  1) Admin deals @mentioning contacts in activity stream trying to mention a non-admin       Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }

     Failure/Error: Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }
       expected to find css ".tribute-cntainer li" but there were no matches

     [Screenshot]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}_152.png
[Screenshot HTML]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}_152.html

I guess this could be considered more correct though:

Failures:

  1) Admin deals @mentioning contacts in activity stream trying to mention a non-admin       Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }

     Failure/Error: Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }
       expected to find css ".tribute-cntainer li" but there were no matches
     
          [Screenshot]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}_314.png
     [Screenshot HTML]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}_314.html

[agrobbin]

I think it'd be great if either of those was the output! However, the maintenance/implementation burden doesn't quite seem worth the "benefit" of aligning the colons vertically, particularly since the output included in extra_failure_lines is completely arbitrary. That's why I went down the road of at least left-aligning the output of each line in extra_failure_lines.

[JonRowe]

I feel rails is aligning the output given your first example? But somehow an extra newline is being generated?

[agrobbin]

Rails is not doing any kind of output-massaging, it's simply doing puts "[Screenshot]: ..." and puts "[Screenshot HTML]: ...", but because rspec-rails is right now doing myio.string here, the first line is properly indented, while any subsequent lines in the output do not properly indent (since they're all considered as one extra failure line, with \ns separating each actual line of output).

Sorry for any confusion, I hope this additional contexts helps explain my proposed change here!

[JonRowe]

Ah I follow now 😂, so we're looking for:

Failures:

  1) Admin deals @mentioning contacts in activity stream trying to mention a non-admin       Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }

     Failure/Error: Then { expect(deal_page).to have_css('.tribute-cntainer li', text: 'John Admin') }
       expected to find css ".tribute-cntainer li" but there were no matches

     [Screenshot]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}_152.png
     [Screenshot HTML]: /Users/foo/Work/bar/tmp/screenshots/failures_r_spec_example_groups_admin_deals_mentioning_contacts_in_activity_stream_trying_to_mention_a_non_admin_______then_{_expect(deal_page)_to_have_css(__tribute_cntainer_li___text___john_admin_)_}_152.html

[agrobbin]

Yep, that's the ideal! I don't think rspec-rails should be doing too much to try and fix any weird additional newlines from Rails, but having each separate puts appropriately indented seems reasonable.

Assuming you're 👍 on that, do you have a recommendation on the ideal approach to test this? Thanks!

[JonRowe]

Something along these lines will get you closer:

describe '#after' do
  it 'sets the :extra_failure_lines metadata to an array of STDOUT lines' do
    group = RSpec::Core::ExampleGroup.describe do
      include SystemExampleGroup

      before { driven_by(:selenium) }
    end
    example = group.it("fails") { fail }
    group.run

    expect(example.metadata[:extra_failure_lines]).to eq(...output)
  end
end

You need to run the example and trick it into using one of its hooks, maybe a custom driver that returns dummy screenshot urls?

[agrobbin]

Sorry for the delay here, @JonRowe! Have had a busy couple of weeks, but finally managed to come back to this and add a test. Just 1 remaining question for you, then this should be good to go. Thanks for all of your help!

[JonRowe]

Thanks!

[agrobbin]

Awesome, thanks again for all your help @JonRowe!