RSpec ignores other files when line numbers are specified #952

Closed
garybernhardt opened this Issue Jun 22, 2013 · 15 comments

Comments

Projects
None yet
5 participants
@garybernhardt
Contributor

garybernhardt commented Jun 22, 2013

I have a simple spec:

$ cat temp_spec.rb
describe "something" do
  it "example 1" do end
  it "example 2" do end
end

Running by line number works just fine:

$ rspec -fd temp_spec.rb:2
Run options: include {:locations=>{"./temp_spec.rb"=>[2]}}

something
  example 1

Finished in 0.0004 seconds
1 example, 0 failures

But if I want to run just that line, then the rest of the file, RSpec disregards the second path:

$ rspec -fd temp_spec.rb:2 temp_spec.rb
Run options: include {:locations=>{"./temp_spec.rb"=>[2]}}

something
  example 1

Finished in 0.00035 seconds
1 example, 0 failures

This command makes sense in conjunction with --fail-fast: it means "run this example, then the whole file, assuming the example passed". Well, I think it should mean that, at least!

The same thing happens even if the second filename is different from the first. I create a second spec file:

$ rspec -fd other_spec_file.rb

other thing
  example 3

Finished in 0.00039 seconds
1 example, 0 failures

and try to run one line from the first file, then the entire second file:

$ rspec -fd temp_spec.rb:2 other_spec_file.rb
Run options: include {:locations=>{"./temp_spec.rb"=>[2]}}

something
  example 1

Finished in 0.00034 seconds
1 example, 0 failures

No joy. :(

@samphippen

This comment has been minimized.

Show comment Hide comment
@samphippen

samphippen Jun 22, 2013

Owner

@garybernhardt thanks for the bug report! The problem here is that the filter manager applies its filters to every example group. The location filter you give above then reduces it down to a single example no matter which files are passed. The other problem here is that the list of files is run through uniq so that with the two runs of the same file we're only ever going to see it once. Part of the reason for the uniq call is when two globs hit the same file twice.

@myronmarston I guess if a file is added explicitly it could get a location filter matching every line, that'd at least solve the second sample. The first one is a little more complicated. Do you have any thoughts?

Owner

samphippen commented Jun 22, 2013

@garybernhardt thanks for the bug report! The problem here is that the filter manager applies its filters to every example group. The location filter you give above then reduces it down to a single example no matter which files are passed. The other problem here is that the list of files is run through uniq so that with the two runs of the same file we're only ever going to see it once. Part of the reason for the uniq call is when two globs hit the same file twice.

@myronmarston I guess if a file is added explicitly it could get a location filter matching every line, that'd at least solve the second sample. The first one is a little more complicated. Do you have any thoughts?

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Jul 13, 2013

Owner

I think this is something we can improve in RSpec 3 (which we've already started on). I think it make sense to change location filters so they apply only to the specified file. Then your rspec -fd temp_spec.rb:2 other_spec_file.rb should work to run one example from temp_spec and all examples from other_spec_file.

Your rspec -fd temp_spec.rb:2 temp_spec.rb example is trickier though: you intend it to control the order examples are run in, but there's a separate API for that that's not easily made compatible with that. By default, specs are run in the order they are defined, and there is a --random option for running them in a random order (with a random seed printed off at the end). Files are loaded in a consistent order (alphabetical, but that's an implementation detail) to help maintain consistent ordering when specifying a particular --seed and a subset of the specs (by passing a directory or file as CLI args) -- otherwise, when you start bisecting a failing test due to an ordering dependency, the order may not be consistent if you didn't pass the files or directories in the same order they were loaded originally. If you want more fine-grained control over the order things run, there's an API for that. You could probably put some logic in that API's callback that inspects ARGV to see what the order of files were, and orders the examples and groups accordingly. We could potentially even add a --order passed that would do that for you (e.g. sets the based on the passed file/dir list). Not sure I like the name "passed", though, and I don't know how widely used it would be.

Owner

myronmarston commented Jul 13, 2013

I think this is something we can improve in RSpec 3 (which we've already started on). I think it make sense to change location filters so they apply only to the specified file. Then your rspec -fd temp_spec.rb:2 other_spec_file.rb should work to run one example from temp_spec and all examples from other_spec_file.

Your rspec -fd temp_spec.rb:2 temp_spec.rb example is trickier though: you intend it to control the order examples are run in, but there's a separate API for that that's not easily made compatible with that. By default, specs are run in the order they are defined, and there is a --random option for running them in a random order (with a random seed printed off at the end). Files are loaded in a consistent order (alphabetical, but that's an implementation detail) to help maintain consistent ordering when specifying a particular --seed and a subset of the specs (by passing a directory or file as CLI args) -- otherwise, when you start bisecting a failing test due to an ordering dependency, the order may not be consistent if you didn't pass the files or directories in the same order they were loaded originally. If you want more fine-grained control over the order things run, there's an API for that. You could probably put some logic in that API's callback that inspects ARGV to see what the order of files were, and orders the examples and groups accordingly. We could potentially even add a --order passed that would do that for you (e.g. sets the based on the passed file/dir list). Not sure I like the name "passed", though, and I don't know how widely used it would be.

@907th 907th referenced this issue in guard/guard-rspec Nov 26, 2013

Merged

Respect guard notifier + Many more refactorings #217

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Nov 26, 2013

Contributor

Hi @myronmarston do you think it'll be fixed in 2.x release? It cause a little issue with a guard-rspec new option (guard/guard-rspec#217 (comment)).

Contributor

thibaudgg commented Nov 26, 2013

Hi @myronmarston do you think it'll be fixed in 2.x release? It cause a little issue with a guard-rspec new option (guard/guard-rspec#217 (comment)).

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Nov 26, 2013

Owner

Hi @myronmarston do you think it'll be fixed in 2.x release?

Hopefully. I know @soulcutter wants to look into this. No promises, though :).

Owner

myronmarston commented Nov 26, 2013

Hi @myronmarston do you think it'll be fixed in 2.x release?

Hopefully. I know @soulcutter wants to look into this. No promises, though :).

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Nov 26, 2013

Contributor

Ok thanks!

Contributor

thibaudgg commented Nov 26, 2013

Ok thanks!

@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Nov 26, 2013

Owner

This is more complex than perhaps we realise, we currently consider locations to be a standalone filter, which means it override all other filters. "Fixing" the processing so that extra files are included when line numbers are omitted is actually a breaking change because it changes the way filters are run within those files.

Owner

JonRowe commented Nov 26, 2013

This is more complex than perhaps we realise, we currently consider locations to be a standalone filter, which means it override all other filters. "Fixing" the processing so that extra files are included when line numbers are omitted is actually a breaking change because it changes the way filters are run within those files.

@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Nov 26, 2013

Owner

For reference I have tried implementing this two ways:

  • Interpreting no line number as a '*' wildcard and applying the filter thusly

Resulted in the desired file behaviour but also triggered specs that were previously filtered out by :if => etc

  • Fixing a potential issue in the metadata filter application that would treat an empty line number array as the entire file

Again resulted in the desired file behaviour but also triggered specs that were previously filtered out by :if => etc

We could change the filter so that it isn't standalone, but then that would break the expected line number behaviour.

Owner

JonRowe commented Nov 26, 2013

For reference I have tried implementing this two ways:

  • Interpreting no line number as a '*' wildcard and applying the filter thusly

Resulted in the desired file behaviour but also triggered specs that were previously filtered out by :if => etc

  • Fixing a potential issue in the metadata filter application that would treat an empty line number array as the entire file

Again resulted in the desired file behaviour but also triggered specs that were previously filtered out by :if => etc

We could change the filter so that it isn't standalone, but then that would break the expected line number behaviour.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Nov 27, 2013

Owner

I think we're modeling line number filters wrong. Currently it's a global standalone filter, but line numbers really only make sense in the context of a given file. I think we should model things as a list of SpecLocation objects. A file name (w/ no attached line number) can be a spec location. A file:line_num can be a spec location. If any spec locations have been specified, then only those specs whose source is in one if the spec locations are run.

One consequence of this is that we would probably drop support for --line_number, but I think that makes sense: to me, it only makes sense when paired with a file name.

Owner

myronmarston commented Nov 27, 2013

I think we're modeling line number filters wrong. Currently it's a global standalone filter, but line numbers really only make sense in the context of a given file. I think we should model things as a list of SpecLocation objects. A file name (w/ no attached line number) can be a spec location. A file:line_num can be a spec location. If any spec locations have been specified, then only those specs whose source is in one if the spec locations are run.

One consequence of this is that we would probably drop support for --line_number, but I think that makes sense: to me, it only makes sense when paired with a file name.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Nov 27, 2013

Contributor

One consequence of this is that we would probably drop support for --line_number, but I think that makes sense: to me, it only makes sense when paired with a file name.

Yes I agree.

Contributor

thibaudgg commented Nov 27, 2013

One consequence of this is that we would probably drop support for --line_number, but I think that makes sense: to me, it only makes sense when paired with a file name.

Yes I agree.

@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Nov 27, 2013

Owner

One consequence of this is that we would probably drop support for --line_number

Agree but I don't think we can backport any significant changes to this behaviour, we also still need to address what happens when you give an entire file as a location, does that override other filters? I think it shouldn't, but I think file_number:location still should.

Owner

JonRowe commented Nov 27, 2013

One consequence of this is that we would probably drop support for --line_number

Agree but I don't think we can backport any significant changes to this behaviour, we also still need to address what happens when you give an entire file as a location, does that override other filters? I think it shouldn't, but I think file_number:location still should.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Nov 27, 2013

Owner

Agree but I don't think we can backport any significant changes to this behaviour

I'm not sure what you mean. We're talking about changes for 3.0, and I don't think we'll backport them to prior versions.

we also still need to address what happens when you give an entire file as a location, does that override other filters? I think it shouldn't, but I think file_number:location still should.

I agree.

Owner

myronmarston commented Nov 27, 2013

Agree but I don't think we can backport any significant changes to this behaviour

I'm not sure what you mean. We're talking about changes for 3.0, and I don't think we'll backport them to prior versions.

we also still need to address what happens when you give an entire file as a location, does that override other filters? I think it shouldn't, but I think file_number:location still should.

I agree.

@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Nov 27, 2013

Owner

I'm not sure what you mean.

Specifically I mean it's not feasible to fix this issue for 2.x as was suggested as a possibility, just clarifying :)

Hi @myronmarston do you think it'll be fixed in 2.x release?

Hopefully. I know @soulcutter wants to look into this. No promises, though :).

Owner

JonRowe commented Nov 27, 2013

I'm not sure what you mean.

Specifically I mean it's not feasible to fix this issue for 2.x as was suggested as a possibility, just clarifying :)

Hi @myronmarston do you think it'll be fixed in 2.x release?

Hopefully. I know @soulcutter wants to look into this. No promises, though :).

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Nov 27, 2013

Owner

Hmm, somehow I missed the 2.x and thought it said 3.x.

I don't think we'll backport this to 2.x. It'll be a 3.x only change. Sorry for wrongly saying otherwise, @thibaudgg.

Owner

myronmarston commented Nov 27, 2013

Hmm, somehow I missed the 2.x and thought it said 3.x.

I don't think we'll backport this to 2.x. It'll be a 3.x only change. Sorry for wrongly saying otherwise, @thibaudgg.

@thibaudgg

This comment has been minimized.

Show comment Hide comment
@thibaudgg

thibaudgg Nov 28, 2013

Contributor

Ok no problem.

Contributor

thibaudgg commented Nov 28, 2013

Ok no problem.

myronmarston added a commit that referenced this issue Mar 20, 2014

Remove `--line-number` option.
CLI filtering options are meant to globally apply
but `--line-number` really only makes sense in the
context of a particular file.

rspec path/to/spec.rb:12:23 is a better way to
filter to an example based on the line.

Fixes #1243 and paves the way for #952.

@myronmarston myronmarston modified the milestones: Post 3.0, 3.0 Mar 24, 2014

myronmarston added a commit that referenced this issue Jan 8, 2015

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Jan 8, 2015

Owner

Sorry for the long delay in addressing this, @garybernhardt, but there's a fix in #1839 if you want to give that a shot. Note that we haven't implemented the ordering semantic you mentioned you would like as the file:num form is only meant to provide filtering, but it will allow you to mix filtered and unfiltered files, which wasn't possible before as you noted.

Owner

myronmarston commented Jan 8, 2015

Sorry for the long delay in addressing this, @garybernhardt, but there's a fix in #1839 if you want to give that a shot. Note that we haven't implemented the ordering semantic you mentioned you would like as the file:num form is only meant to provide filtering, but it will allow you to mix filtered and unfiltered files, which wasn't possible before as you noted.

myronmarston added a commit that referenced this issue Jan 8, 2015

@JonRowe JonRowe closed this in #1839 Jan 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment