Skip to content
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

3.2.2 breaks line number filter when external shared examples are included #1961

Closed
ben-axnick opened this issue May 11, 2015 · 7 comments · Fixed by #1963
Closed

3.2.2 breaks line number filter when external shared examples are included #1961

ben-axnick opened this issue May 11, 2015 · 7 comments · Fixed by #1963

Comments

@ben-axnick
Copy link

In going from rspec-core 3.2.1 to rspec-core 3.2.2 (included into the bundle via rspec-rails), I've noticed that CLI commands in the form:

> bundle exec foo.rb:123

where 123 is the start/inner line of an it block no longer behave as expected. In 3.2.1, only that single test would be run. In 3.2.2 all it_behaves_like blocks in the file get executed before running the indicated test. This can add significantly to the amount of time needed to interactively run tests if there are many tests defined inside the shared_examples block that it_behaves_like refers to.

The bug/change is still present in 3.2.3 but I have not tested against master.

Issue

Appears to occur when shared examples are required from an external file.

Reproducing

Setup files as below (or git clone https://github.com/bentheax/sample-rspec-for-bug-report.git) and run

> bundle exec rspec spec/minimal_spec.rb:7

Gemfile

source "https://rubygems.org"

gem "rspec"
gem "rspec-core", "=3.2.2"

Gemfile.lock

GEM
  remote: https://rubygems.org/
  specs:
    diff-lcs (1.2.5)
    rspec (3.2.0)
      rspec-core (~> 3.2.0)
      rspec-expectations (~> 3.2.0)
      rspec-mocks (~> 3.2.0)
    rspec-core (3.2.2)
      rspec-support (~> 3.2.0)
    rspec-expectations (3.2.1)
      diff-lcs (>= 1.2.0, < 2.0)
      rspec-support (~> 3.2.0)
    rspec-mocks (3.2.1)
      diff-lcs (>= 1.2.0, < 2.0)
      rspec-support (~> 3.2.0)
    rspec-support (3.2.2)

PLATFORMS
  ruby

DEPENDENCIES
  rspec
  rspec-core (= 3.2.2)

spec/a_test_that_always_runs_external_examples.rb

shared_examples "a test that always runs external examples" do
  it "runs me, for instance" do
    raise "this line should never be executed"
  end
end

spec/minimal_spec.rb

require_relative "./a_test_that_always_runs_external_examples"

describe "rspec_with_line_number" do
  it_behaves_like "a test that always runs external examples"

  it "has a test that I'd like to run on its own" do
    expect(1).to eq(1)
  end
end

Output

> bundle exec rspec -fd spec/minimal_spec.rb:7
Run options: include {:locations=>{"./spec/minimal_spec.rb"=>[7]}}

rspec_with_line_number
  has a test that I'd like to run on its own
  behaves like a test that always runs external examples
    runs me, for instance (FAILED - 1)

Failures:

  1) rspec_with_line_number behaves like a test that always runs external examples runs me, for instance
     Failure/Error: raise "this line should never be executed"
     RuntimeError:
       this line should never be executed
     Shared Example Group: "a test that always runs external examples" called from ./spec/minimal_spec.rb:4
     # ./spec/a_test_that_always_runs_external_examples.rb:3:in `block (2 levels) in <top (required)>'

Finished in 0.00151 seconds (files took 0.0792 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./spec/minimal_spec.rb:4 # rspec_with_line_number behaves like a test that always runs external examples runs me, for instance
@ben-axnick ben-axnick changed the title 3.2.2 breaks line number handling 3.2.2 breaks line number filter in some cases May 11, 2015
@ben-axnick
Copy link
Author

I'm unlikely to reproduce the issue tonight, will close until I have a minimal sample file that can reproduce the issue.

@ben-axnick
Copy link
Author

I managed to reproduce it tonight, after all.

If it's bad / unsupported / edge case let me know and I'll follow up accordingly. If I've stumbled upon an actual bug, I'm happy to make a PR for a test+fix.

@ben-axnick ben-axnick reopened this May 11, 2015
ben-axnick pushed a commit to ben-axnick/sample-rspec-for-bug-report that referenced this issue May 11, 2015
@ben-axnick ben-axnick changed the title 3.2.2 breaks line number filter in some cases 3.2.2 breaks line number filter when external shared examples are included May 11, 2015
@myronmarston
Copy link
Member

This is definitely a regression. Thanks for putting together the reproduction!

The regression is caused by 0a4937b / 8772cf5 (the first commit caused the regression in master (3.3.0.pre) whereas the latter caused it in 3.2.2). When you run a command line rspec --tag foo 1_spec.rb:10 2_spec.rb 3_spec.rb, the line number filter (10) should only apply to 1_spec.rb where as the tag filter should only apply to 2_spec.rb and 3_spec.rb. So line number filters are treated as being scoped to the specific file they are attached to and should not apply to other spec files.

In your case, you've got specs defined in a shared example group file, and so the line number filtering isn't applying and they are running in spite of the fact they shouldn't. I'm not sure what the solution should be but here are a few thoughts:

  • We can differentiate between spec files and non-spec files that define examples via RSpec.configuration.loaded_spec_files.
  • In master, there's a new metadata entry for every example and group that may be helpful here: :rerun_file_path. Whereas :file_path is the location on disk where the example is defined, :rerun_file_path is the file that needs to be passed as an argument to rspec in order to run or rerun a particular example. For the shared example in your spec, the :file_path would be spec/a_test_that_always_runs_external_examples.rb where as :rerun_file_path would be spec/minimal_spec.rb.
  • For line number filtering, it can only apply to examples defined in the specified file (obviously) but examples in other files that have the specified file as the :rerun_file_path should not run or be available to filter by a tag or some other mechanism.

Does that help point you in a direction for trying to work up a PR?

@myronmarston
Copy link
Member

One other thing...if you use :rerun_file_path, that's available in master but not in 3-2-maintenance, which means fixing this on 3-2-maintenance would be tricky. Usually we like to backport bug fixes to the maintenance branch and get a patch release out but in this case that's not simple, and 3.3.0 is just around the corner, so I think it's sufficient to fix it just in master.

@ben-axnick
Copy link
Author

Does that help point you in a direction for trying to work up a PR?

Looks like all the information I should need to get started, thanks. I'll get onto a PR as soon as is practical within the next few days

@ben-axnick
Copy link
Author

@myronmarston

I think a tweak along these lines ben-axnick@01be52a should behave correctly both in cases where only location filters are present, and in cases where tag inclusions/exclusions are also present. At a high level, basically "if the current example is included as part of a location filtered file, but doesn't have its own location filters present, then prune it immediately".

If you agree with the gist of that, I'll clean it up and then get my reproductions directory converted over to a set of specs.

Unfortunately, I did end up using :rerun_file_path, so I'm not sure what the substitute would be for 3-2-maintenance at this stage. Still, if you're happy with just a fix against master, I likely won't investigate that too deeply.

@myronmarston
Copy link
Member

That solution looks fine. I'm not really concerned about backporting to 3-2 so let's just fix it against master.

ben-axnick pushed a commit to ben-axnick/rspec-core that referenced this issue May 15, 2015
Fixes rspec#1961

The issue is that when the rerun file of an example is not taken into account,
it is treated by the filtering mechanism as completely separate file. This
causes issues for location filtering, which causes an implicit exclusion scoped
to its own file. The fix extends the scope of that exclusion to any examples
for which it is the rerun path. i.e. an external shared example that is
`include`d or `require`d.
ben-axnick pushed a commit to ben-axnick/rspec-core that referenced this issue May 15, 2015
ben-axnick pushed a commit to ben-axnick/rspec-core that referenced this issue May 16, 2015
Fixes rspec#1961

The problem with using :absolute_file_path is that it doesn't function
correctly for shared examples in an external file. The filter manager thinks
that there are no location filters set, when actually there are. Since
MetadataFilter already handles location filtering correctly across multiple
nested examples / files, correcting the path that is checked is sufficient to
fix the line filtering issue.
ben-axnick pushed a commit to ben-axnick/rspec-core that referenced this issue May 18, 2015
Fixes rspec#1961

The problem with using :absolute_file_path is that it doesn't function
correctly for shared examples in an external file. The filter manager thinks
that there are no location filters set, when actually there are. Since
MetadataFilter already handles location filtering correctly across multiple
nested examples / files, correcting the path that is checked is sufficient to
fix the line filtering issue.
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this issue Oct 30, 2020
Fixes rspec#1961

The problem with using :absolute_file_path is that it doesn't function
correctly for shared examples in an external file. The filter manager thinks
that there are no location filters set, when actually there are. Since
MetadataFilter already handles location filtering correctly across multiple
nested examples / files, correcting the path that is checked is sufficient to
fix the line filtering issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants