New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract multiple failed lines by parsing Ruby #2083

Merged
merged 10 commits into from Oct 27, 2015

Conversation

Projects
None yet
3 participants
@yujinakayama
Copy link
Member

yujinakayama commented Oct 9, 2015

For rspec/rspec-expectations#790

With the following failing example:

expect('RSpec').to be_a(String)
              .and start_with('R')
              .and end_with('z')

Before

Failures:

  1) failure snippet is an example
     Failure/Error: expect('RSpec').to be_a(String)
       expected "RSpec" to end with "z"
     # ./spec/test_spec.rb:3:in `block (2 levels) in <top (required)>'

After

Failures:

  1) failure snippet is an example
     Failure/Error:
       expect('RSpec').to be_a(String)
                     .and start_with('R')
                     .and end_with('z')

       expected "RSpec" to end with "z"
     # ./spec/test_spec.rb:3:in `block (2 levels) in <top (required)>'

@yujinakayama yujinakayama changed the title [WIP] Extract failed lines by parsing ruby [WIP] Extract multiple failed lines by parsing Ruby Oct 9, 2015

@yujinakayama yujinakayama force-pushed the extract-failed-lines-by-parsing-ruby branch 4 times, most recently from e6c48db to 6ff075b Oct 9, 2015

@@ -59,7 +59,8 @@ Feature: Aggregating Failures
# ./spec/use_block_form_spec.rb:18
# ./spec/use_block_form_spec.rb:10
1.1.1) Failure/Error: expect(response.status).to eq(200)
1.1.1) Failure/Error:
expect(response.status).to eq(200)

This comment has been minimized.

@myronmarston

myronmarston Oct 9, 2015

Member

For multiline snippets it should certainly be started on the next line but for single line snippets I think I prefer it to be on the same line like it was before. It cuts down on unnecessary churn in this PR if you don't have to change all the expectations about the failure formatting and keeping it to one line means the failure output doesn't take up as much vertical space, which is always nice.

How much effort would it be to keep single-line failures on the same line as Failure/Error:?

This comment has been minimized.

@yujinakayama

yujinakayama Oct 10, 2015

Member

Actually that's what I was unsure about. I made the snippets be always on the next line for consistency, but it's not a strong opinion. I'll make change to do so.

By the way there's another concern about putting the snippets on the next line. There's no problem when an RSpec's exception has a message starting with a blank line:

 1.1) Failure/Error:
        expect(response.status).to eq(200)

         expected: 200
              got: 404

         (compared using ==)
       # ./spec/nested_failure_aggregation_spec.rb:7

However it's hard to read when there's no blank line:

 1.2.2) Failure/Error:
          expect(response.headers).to include("Content-Length" => "21")
          expected {"Content-Type" => "text/plain"} to include {"Content-Length" => "21"}
          Diff:
          @@ -1,2 +1,2 @@
          -[{"Content-Length"=>"21"}]
          +"Content-Type" => "text/plain",
        # ./spec/nested_failure_aggregation_spec.rb:11

Maybe we should always put a blank line in ExceptionPresenter and remove the blank line in exception message?

This comment has been minimized.

@yujinakayama

yujinakayama Oct 10, 2015

Member

Oh you had already mentioned it :) #2083 (comment)

# @macro add_setting
# Maximum count of failed source lines to display in the failure reports.
# (default `10`).
add_setting :max_displayed_count_of_failed_lines

This comment has been minimized.

@myronmarston

myronmarston Oct 9, 2015

Member

This is subjective, but I think I slightly prefer max_displayed_failure_line_count as the name for this. What do others think?

This comment has been minimized.

@JonRowe

JonRowe Oct 9, 2015

Member

I prefer max_displayed_failure_line_count

This comment has been minimized.

@yujinakayama

def failure_slash_error_lines
read_failed_lines.tap do |lines|
least_indentation = lines.map { |line| line.match(/^[ \t]*/)[0] }.min

This comment has been minimized.

@myronmarston

myronmarston Oct 9, 2015

Member

I think I'd prefer this as:

least_indentation = lines.map { |line| line[/^[ \t]*/] }.min

Besides being shorter, I suspect that it's faster (though, I haven't benchmarked it). There's no need for it to instantiate the MatchData object and there's not an extra [0] message send.

def failure_slash_error_lines
read_failed_lines.tap do |lines|
least_indentation = lines.map { |line| line.match(/^[ \t]*/)[0] }.min
lines.map! { |line| ' ' + line.gsub(/^#{least_indentation}/, '') }

This comment has been minimized.

@myronmarston

myronmarston Oct 9, 2015

Member

I think this can use sub instead of gsub, which might be faster (as gsub may try to find more matches in the string....) and is more explicit about it doing only one substitution.

Also, would this line be equivalent to this?

lines.map! { |line| line.gsub(/^#{least_indentation}/, '  ') }

It seems odd to strip all of the common indentation, then add back two spaces of indentation, rather than just replacing the common indentation with two spaces.

Or is it necessary to do it in two steps for some reason I'm not seeing?

This comment has been minimized.

@myronmarston

myronmarston Oct 9, 2015

Member

Also, the map! (as opposed to map) call concerns me a little bit -- since the lines array wasn't instantiated locally in this method it's not clear to me from reading the code who else may have a reference to the array and it always makes me nervous to mutate an object that someone else may have a reference to.

@@ -254,7 +271,7 @@ def multiple_exceptions_error?(exception)
end

def multiple_exception_summarizer(exception, prior_detail_formatter, color)
lambda do |example, colorizer, indentation|
lambda do |example, colorizer|

This comment has been minimized.

@myronmarston

myronmarston Oct 9, 2015

Member

Nice to see things getting simpler here with fewer variables needed in these lambdas...

#
# Extracts code snippets by looking at the backtrace of the passed error
# and applies synax highlighting and line numbers using html.
# @private
class SnippetExtractor

This comment has been minimized.

@myronmarston

myronmarston Oct 9, 2015

Member

Your renaming of SnippetExtractor to HtmlSnippetExtractor suggests that maybe we shouldn't add a new class with the name SnippetExtractor because there really isn't such thing as a generic one. Maybe this class would be better named TerminalSnippetExtractor?

This comment has been minimized.

@yujinakayama

yujinakayama Oct 10, 2015

Member

The new SnippetExtractor is generic and not limited for terminal use. Maybe it can be used even in HtmlSnippetExtractor in the future.

This comment has been minimized.

@myronmarston

myronmarston Oct 17, 2015

Member

Good point!

def self.source_from_file(path)
raise_error_if_file_does_not_exist!(path)
@source_cache ||= {}
@source_cache[path] ||= Source.from_file(path)
end

This comment has been minimized.

@myronmarston

myronmarston Oct 9, 2015

Member

I'd prefer we not attach global mutable state to the class like this. Issues I have with it:

  • It can make testing difficult. You have to consciously think about clearing this source cache between tests.
  • I think users expect that all RSpec global state will be cleared when RSpec.reset is called, but this state here will not be cleared as currently implemented. Concretely, this can cause problems when the user uses an alternate runner that keeps the RSpec process around and then triggers it to load and run spec files via a signal or some other mechanism. In such a situation, the source files could change between runs but the cache here would persist and could lead to the wrong snippets being displayed.
  • I think it makes the logic harder to reason about.

Instead, what do you think of this alternate design?

  • Remove the global state from the snippet extractor.
  • Add a Source::Cache class (presumably defined in source.rb) that exposes a source_from_file instance method.
  • Expose a Source::Cache instance off of some other RSpec object that gets automatically cleared when RSpec is reset. I'm not sure the best place but the reporter (e.g. RSpec.configuration.reporter.source_cache) and RSpec.world.source_cache are two possibilities. Neither of those is obviously perfect so there may be a better place I'm not thinking of.

This comment has been minimized.

@yujinakayama

yujinakayama Oct 10, 2015

Member

👍

RSpec.world.source_cache sounds good.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Oct 9, 2015

I'm really excited about this feature, @yujinakayama -- thanks for developing it! I'm not done with my review yet but need to head to bed. For now I wanted to share a few general thoughts:

  • I believe the build is failing due to the issue described in rspec/rspec-dev#137. It would probably be helpful to make the rspec-support PR and get that merged so the build failure goes away.
  • CodeRay, the gem we use to provide syntax highlighting for HTML code snippets also includes a terminal encoder. IMO it would be pretty amazing if we could syntax highlight these snippets in the failure output. It would make them much easier to read. We obviously don't want to require CodeRay as a hard dependency but we can do it like we do the HTML snippets -- use it if we're able to load it, and if we're not, we can add a # Install the coderay gem to get syntax highlighting comment to the bottom of each multi-line snippet. (For single-line snippets I'd like them to stay on the Failure/Error: line as mentioned above so the comment wouldn't fit there). I definitely don't consider adding syntax highlighting to be a merge blocker but it would make this feature even more useful, IMO, and hopefully isn't difficult.
  • Another idea for a future enhancement to this (and definitely not a requirement for this PR): perhaps we could add left-aligned line numbers next to the snippet? Actually, if we do that, that might sway me to put single-line snippets on their own line like you've already done.
  • Is the new code you added lazily loaded only when a failure actually occurs? As much as possible I want RSpec to load as little as possible to get booted and running, and then delay the loading of optional features like this until they are actually used.
  • There's a planned change I haven't yet made (discussed in #1991) that could affect things here: instead of looking only for a failure snippet from a spec file (as the current logic does) I'd like it to show failure snippets from other files, too. Getting "unable to find line" is never useful. That may affect things here since spec file failures are generally single expectation expressions but errors from other files may not have quite as clear snippet boundaries. Any thoughts on how this feature might interact with that change if we ever make it?
  • Are there any gotchas or situations where the snippet extractor doesn't work properly? For example, how does it handle the case where there are multiple expectations on one line but only one failed?
expect(1).to eq(1); expect(2).to eq(1); expect(2).to eq(2)

This is a contrived example obviously...

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Oct 9, 2015

I noticed a case where this didn't work as I expect:

expect {
  expect("foo").to start_with("a").and end_with("z")
}.to fail_with(dedent <<-EOS)
  |   expected "fo" to start with "a"
  |
  |...and:
  |
  |   expected "foo" to end with "z"
EOS

(This is a slightly modified spec from rspec-expectations). The extracted snippet is:

       expect {
         expect("foo").to start_with("a").and end_with("z")
       }.to fail_with(dedent <<-EOS)

Notice it's missing the heredoc. It would be nice if that was included.

In addition, in the full output the snippet is crammed right next to the failure message:

     Failure/Error:
       expect {
         expect("foo").to start_with("a").and end_with("z")
       }.to fail_with(dedent <<-EOS)
       expected RSpec::Expectations::ExpectationNotMetError with "   expected \"fo\" to start with \"a\"\n\n...and:\n\n   expected \"foo\" to end with \"z\"", got #<RSpec::Expectations::ExpectationNotMetError:    expected "foo" to start with "a"

       ...and:

          expected "foo" to end with "z"> with backtrace:
 ...

I think it would be easier if there was an extra line break after the snippet:

     Failure/Error:
       expect {
         expect("foo").to start_with("a").and end_with("z")
       }.to fail_with(dedent <<-EOS)

       expected RSpec::Expectations::ExpectationNotMetError with "   expected \"fo\" to start with \"a\"\n\n...and:\n\n   expected \"foo\" to end with \"z\"", got #<RSpec::Expectations::ExpectationNotMetError:    expected "foo" to start with "a"

       ...and:

          expected "foo" to end with "z"> with backtrace:
 ...

Or perhaps on both sides of the snippet:

     Failure/Error:

       expect {
         expect("foo").to start_with("a").and end_with("z")
       }.to fail_with(dedent <<-EOS)

       expected RSpec::Expectations::ExpectationNotMetError with "   expected \"fo\" to start with \"a\"\n\n...and:\n\n   expected \"foo\" to end with \"z\"", got #<RSpec::Expectations::ExpectationNotMetError:    expected "foo" to start with "a"

       ...and:

          expected "foo" to end with "z"> with backtrace:
...

@yujinakayama yujinakayama force-pushed the extract-failed-lines-by-parsing-ruby branch 4 times, most recently from 032acb1 to 36c4334 Oct 10, 2015

yujinakayama added a commit that referenced this pull request Oct 10, 2015

@yujinakayama yujinakayama force-pushed the extract-failed-lines-by-parsing-ruby branch from 36c4334 to 232ba6b Oct 10, 2015

yujinakayama added a commit that referenced this pull request Oct 10, 2015

@yujinakayama yujinakayama force-pushed the extract-failed-lines-by-parsing-ruby branch 3 times, most recently from 20067be to 62d79b4 Oct 10, 2015

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Oct 13, 2015

@yujinakayama -- I went ahead and addressed #1991 via #2088 as that will help to give 3.4 a nice "vastly improved failure output" theme when combined with this PR and rspec/rspec-expectations#859. I don't think it complicates this PR but there might be a slight merge conflict when one or the other PR gets merged.

@yujinakayama yujinakayama force-pushed the extract-failed-lines-by-parsing-ruby branch from 62d79b4 to b6973b9 Oct 14, 2015

@yujinakayama

This comment has been minimized.

Copy link
Member

yujinakayama commented Oct 14, 2015

Now I'm trying the improve the format of message lines and thinking of it in various cases.

Here are some failure messages with the current implementation in this branch:

  1) failure message format is a single line expectation with a single line failure message
     Failure/Error: expect(1).to be_a(String)
       expected 1 to be a kind of String
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  2) failure message format is a multi-line expectation with a single line failure message
     Failure/Error:
       expect(1)
         .to be_a(String)
       expected 1 to be a kind of String
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  3) failure message format is a single line expectation with a multi-line failure message
     Failure/Error: expect(1).to eq(2)

       expected: 2
            got: 1

       (compared using ==)
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  4) failure message format is a multi-line expectation with a multi-line failure message
     Failure/Error:
       expect(1)
         .to eq(2)

       expected: 2
            got: 1

       (compared using ==)
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

The failure 2 is obviously unreadble. Also, the formats are inconsistent a bit.

To address these issues, I'm thinking of the following two formats:

A: Less lines when possible

  • Put the snippet on the same line with the Failure/Error: when it's single line
  • Insert a blank line after the snippet only when the it's multiline
  1) failure message format is a single line expectation with a single line failure message
     Failure/Error: expect(1).to be_a(String)
       expected 1 to be a kind of String
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  2) failure message format is a multi-line expectation with a single line failure message
     Failure/Error:
       expect(1)
         .to be_a(String)

       expected 1 to be a kind of String
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  3) failure message format is a single line expectation with a multi-line failure message
     Failure/Error: expect(1).to eq(2)
       expected: 2
            got: 1

       (compared using ==)
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  4) failure message format is a multi-line expectation with a multi-line failure message
     Failure/Error:
       expect(1)
         .to eq(2)

       expected: 2
            got: 1

       (compared using ==)
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

B: Consistent format

  • Always put the snippet after the Failure/Error: line
  • Always insert a blank line after the snippet
  1) failure message format is a single line expectation with a single line failure message
     Failure/Error:
       expect(1).to be_a(String)

       expected 1 to be a kind of String
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  2) failure message format is a multi-line expectation with a single line failure message
     Failure/Error:
       expect(1)
         .to be_a(String)

       expected 1 to be a kind of String
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  3) failure message format is a single line expectation with a multi-line failure message
     Failure/Error:
       expect(1).to eq(2)

       expected: 2
            got: 1

       (compared using ==)
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

  4) failure message format is a multi-line expectation with a multi-line failure message
     Failure/Error:
       expect(1)
         .to eq(2)

       expected: 2
            got: 1

       (compared using ==)
     # /Users/me/Projects/rspec-dev/repos/rspec-support/lib/rspec/support.rb:87:in `block in <module:Support>'

What do you think?

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Oct 14, 2015

How about option (C):

  • Put the snippet on the same line with the Failure/Error: when it's single line
  • Insert a blank line between the snippet and failure message when either are multiline strings.

(In other words, the only time there wouldn't be a blank line is when both are single line strings).

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Oct 14, 2015

BTW, I merged #2088 so you may want to rebase.

@yujinakayama yujinakayama force-pushed the extract-failed-lines-by-parsing-ruby branch 3 times, most recently from be71fe5 to edc8e4b Oct 23, 2015

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Oct 25, 2015

❤️ This looks great to me! It'll need a changelog entry but that can happen post-merge. Want to do the honors, @yujinakayama?

@yujinakayama yujinakayama force-pushed the extract-failed-lines-by-parsing-ruby branch from 650411d to f37093b Oct 27, 2015

@yujinakayama

This comment has been minimized.

Copy link
Member

yujinakayama commented Oct 27, 2015

Added changelog entries 👌

@JonRowe

This comment has been minimized.

Copy link
Member

JonRowe commented Oct 27, 2015

Merge when green! :)

@yujinakayama

This comment has been minimized.

Copy link
Member

yujinakayama commented Oct 27, 2015

BTW I'll try code highlight with CodeRay and hopefully it'll be merged before 3.4 release

yujinakayama added a commit that referenced this pull request Oct 27, 2015

Merge pull request #2083 from rspec/extract-failed-lines-by-parsing-ruby
Extract multiple failed lines by parsing Ruby

@yujinakayama yujinakayama merged commit eedf9a9 into master Oct 27, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yujinakayama yujinakayama deleted the extract-failed-lines-by-parsing-ruby branch Oct 27, 2015

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Oct 27, 2015

BTW I'll try code highlight with CodeRay and hopefully it'll be merged before 3.4 release

Sounds great! FWIW, I played around with it a bit a few days ago and I pushed my spike up in 170b28e. Feel free to use any (or none) of that in whatever you come up with.

myronmarston added a commit to rspec/rspec-expectations that referenced this pull request Nov 12, 2015

Fix cukes.
The output formatting changed in rspec/rspec-core#2083,
but we didn’t realize it broke the cukes here.

myronmarston added a commit to rspec/rspec-expectations that referenced this pull request Nov 12, 2015

Fix cukes.
The output formatting changed in rspec/rspec-core#2083,
but we didn’t realize it broke the cukes here.

myronmarston added a commit to rspec/rspec-expectations that referenced this pull request Nov 12, 2015

Fix cukes.
The output formatting changed in rspec/rspec-core#2083,
but we didn’t realize it broke the cukes here.

myronmarston added a commit to myronmarston/rspec-given that referenced this pull request Jan 12, 2016

Fix failures in `failing_messages_spec.rb`.
RSpec changed how it detects source lines to print in
rspec/rspec-core#2088, which necessitates us configuring
`project_source_dirs` for our specs. (This should not affect
end-users).

In addition, we now print multi-line code snippets as of
rspec/rspec-core#2083. Multiline snippets are not printed
on the same line as `Failure/Error:` so our regex has to
be updated to accommodate a line break.

searls added a commit to rspec-given/rspec-given that referenced this pull request Jan 14, 2016

Fix failures in `failing_messages_spec.rb`.
RSpec changed how it detects source lines to print in
rspec/rspec-core#2088, which necessitates us configuring
`project_source_dirs` for our specs. (This should not affect
end-users).

In addition, we now print multi-line code snippets as of
rspec/rspec-core#2083. Multiline snippets are not printed
on the same line as `Failure/Error:` so our regex has to
be updated to accommodate a line break.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment