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

Mixing files and ids doesn't work correctly #1920

Closed
myronmarston opened this Issue Apr 3, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@myronmarston
Copy link
Member

myronmarston commented Apr 3, 2015

I'm seeing that rspec path/to/spec_1.rb[1:1] path/to/spec_2.rb does not work properly -- it selects the appropriate example from spec_1.rb but doesn't run anything from spec_2.rb like it should.

@myronmarston myronmarston self-assigned this Apr 3, 2015

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Apr 4, 2015

More info: this bug only appears to happen when you've got filter_run :focus configured.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Apr 4, 2015

This is more complex than I first realized and not limited to mixing ids and files -- it also applies to mixing locations (e.g. file_name:line_num) and files, and as such, is not a new issue. So I don't consider it a release blocker anymore but would still like to get it addressed in the near term.

To describe the problem in more detail:

  • We treat location and id filters as being scoped to a given file, and as such they don't apply to other files. Thus, when you run rspec path/to/spec_1.rb[1:1] path/to/spec_2.rb it should run the first example from the first group in spec_1.rb and all the specs in spec_2.rb.
  • This works under the default RSpec configuration (which is why our existing specs didn't catch it).
  • It does not work when you've configured filter_run :focus and run_all_when_everything_filtered (which is very common, given that it's in the generated spec_helper.rb.
  • The reason it doesn't work is because run_all_when_everything_filtered doesn't do anything in this case. Examples from spec_1.rb are filtered (i.e. all examples but the 1st one in the 1st group), and thus the :focus filter is left in effect and it applies to the specs in spec_2.rb, and since none of them are filtered, none of them run.
  • The :focus filtering does not apply to the spec that is selected from spec_1.rb via the ID from the command line. We decided that when you explicitly select an example from the command line that should take precedence and the example should run.
  • I think the fix will be to move away from run_all_when_everything_filtered to a new config option with an improved semantic -- something like ignore_unmatched_filters. The idea of this new config option is that when set, configured filters would only be in effect if they apply to at least one example or group. So the :focus filtering would only apply if something is tagged with :focus. Unlike run_all_when_everything_filtered, when ignore_unmatched_filters kicks in, it would only ignore the specific filters that are unmatched; other matched filters would be left alone and in effect.
  • I've thought a bit about whether or not run_all_when_everything_filtered should simply be changed to do what I've describe above for ignore_unmatched_filters. However, if we make that change, then I don't think that it's behavior aligns with its name at all, and as such it would be confusing. So I'm thinking that we should instead do the following:
    • Introduce the new ignore_unmatched_filters option
    • Update the generated spec_helper.rb to set ignore_unmatched_filters rather than run_all_when_everything_filtered
    • Consider run_all_when_everything_filtered deprecated (but not necessarily emit a deprecation warning yet -- that can probably wait until 3.99)
    • Remove run_all_when_everything_filtered in 4.0
  • Alternately, we could keep run_all_when_everything_filtered around and not consider it depecated. However, I can't think of a situation where a user would rather have it than ignore_unmatched_filters.

Thoughts from @rspec/rspec ?

@xaviershay

This comment has been minimized.

Copy link
Member

xaviershay commented Apr 7, 2015

rather than me guessing, could you provide some expected inputs/outputs as to what is current vs desired behaviour? i.e.

rspec spec1.rb[1,1] spec_with_focused_example.rb --focus
should run: focused example from spec_with_focused_example.rb

Sorry I'm not as familiar with the behaviour here.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Apr 7, 2015

Line number and id filters are considered to be "scoped" filters in that they are attached to a specific file and don't apply to examples in other files. Thus, w/o any other filters in affect:

  • rspec spec1.rb[1:1] spec2.rb runs the specified example from spec1.rb and all examples from spec2.rb.
  • rspec spec1.rb:13 spec2.rb is similar: runs the specified example (via line number instead of id) from spec1.rb and all examples from spec2.rb.

When you throw focus filtering into the mix (e.g. under the typical configuration):

  • rspec spec1.rb[1:1] spec2.rb and rspec spec1.rb:13 spec2.rb, when no examples are tagged with :focus, runs just the specified example from spec1.rb and nothing from spec2.rb. (This is the bug that this issue is about).
  • rspec spec1.rb[1:1] spec2.rb and rspec spec1.rb:13 spec2.rb, when some examples are tagged with :focus in spec2.rb, runs the specified example from spec1.rb and the focused examples from spec2.rb. (This is correct, desired behavior).
  • rspec spec1.rb[1:1] spec2.rb and rspec spec1.rb:13 spec2.rb, when some examples are tagged with :focus in spec1.rb, ignores the focused examples since the user has selected a specific example from spec1.rb and CLI-specified examples take precedence. Again, this is correct, desired behavior.

Does that help, @xaviershay?

@xaviershay

This comment has been minimized.

Copy link
Member

xaviershay commented Apr 8, 2015

fantastic, thank you I get it now. I'll come back later on an see if I can suggest anything.

@JonRowe

This comment has been minimized.

Copy link
Member

JonRowe commented Apr 9, 2015

This is more complex than I first realized and not limited to mixing ids and files -- it also applies to mixing locations (e.g. file_name:line_num) and files, and as such, is not a new issue. So I don't consider it a release blocker anymore but would still like to get it addressed in the near term.

To describe the problem in more detail:

  • We treat location and id filters as being scoped to a given file, and as such they don't apply to other files. Thus, when you run rspec path/to/spec_1.rb[1:1] path/to/spec_2.rb it should run the first example from the first group in spec_1.rb and all the specs in spec_2.rb.
  • This works under the default RSpec configuration (which is why our existing specs didn't catch it).
  • It does not work when you've configured filter_run :focus and run_all_when_everything_filtered (which is very common, given that it's in the generated spec_helper.rb.

So I think we should fix this, line numbers / ids etc aren't meant to have the same semantics as metadata filtering so the behaviour is incorrect in my opinion

  • I think the fix will be to move away from run_all_when_everything_filtered to a new config option with an improved semantic -- something like ignore_unmatched_filters. The idea of this new config option is that when set, configured filters would only be in effect if they apply to at least one example or group. So the :focus filtering would only apply if something is tagged with :focus. Unlike run_all_when_everything_filtered, when ignore_unmatched_filters kicks in, it would only ignore the specific filters that are unmatched; other matched filters would be left alone and in effect.
  • I've thought a bit about whether or not run_all_when_everything_filtered should simply be changed to do what I've describe above for ignore_unmatched_filters. However, if we make that change, then I don't think that it's behavior aligns with its name at all, and as such it would be confusing. So I'm thinking that we should instead do the following:
    • Introduce the new ignore_unmatched_filters option
    • Update the generated spec_helper.rb to set ignore_unmatched_filters rather than run_all_when_everything_filtered
    • Consider run_all_when_everything_filtered deprecated (but not necessarily emit a deprecation warning yet -- that can probably wait until 3.99)
    • Remove run_all_when_everything_filtered in 4.0

I guess this is semantically the same concept, I'm on board as long as we make it clear why we're ignoring the filter.

  • Alternately, we could keep run_all_when_everything_filtered around and not consider it depecated. However, I can't think of a situation where a user would rather have it than ignore_unmatched_filters.

I think we should only have one way of doing things, so I think we should deprecate run_all_when_everything_filtered if we opt for ignore_unmatched_filters

@xaviershay

This comment has been minimized.

Copy link
Member

xaviershay commented Apr 9, 2015

Reading this over again now that I understand it better, I agree deprecating run_all_with_everything_filtered in the way myron suggested.

@igor47

This comment has been minimized.

Copy link

igor47 commented Nov 16, 2015

just ran into this issue. my workaround was to remove config.filter_run :focus and config.run_all_when_everything_filtered = true from my spec_helper and to ask people to run rspec -t focus when they wanted the old behavior. is that a reasonable workaround, or is there a simpler one?

agree that ignore_unmatched_filters makes more sense as a config option.

@myronmarston

This comment has been minimized.

Copy link
Member

myronmarston commented Feb 9, 2016

I've been thinking about this some more, and I've realized that while the solution I proposed above -- ignore_unmatched_filters -- solves this issue, it creates other issues. rspec --only-failures currently runs nothing if there are no failures to run. A new ignore_unmatched_filters option would presumably ignore the filter created by --only-failures and run all examples instead. Similarly, if someone runs rspec --tag my_tag_mispelled do they really want it to be ignored? I doubt it.

What I've realized is that a global option to ignore is generally problematic. IIRC, run_all_when_everything_filtered was first introduced as a way to leave focus filtering in place and that's really the only use case I see for it. I think that means that we really want a way to configure just the :focus filtering to be ignored if there are no matching examples, instead of configuring all filters to be ignored.

So now I'm thinking we should introduce a new config option that is a sibling of filter_run. Something like conditonally_filter_run or filter_run_if_any_match or similar. Whereas filter_run would always apply (assuming run_all_when_everything_filtered is not set), this new config option would define a filter that would only apply if any examples match the metadata.

Besides fixing this issue and avoiding an option that globally changes how filters work in undesirable ways, it also reduces the number of lines of config needed to get focus filtering to just one.

Thoughts? And if we go this direction, what should we call the new config option?

@JonRowe

This comment has been minimized.

Copy link
Member

JonRowe commented Feb 11, 2016

I like this, and we could then deprecate run_all_when_everything_filtered before RSpec 4? My vote for the name is filter_run_when_matching

@chrisarcand

This comment has been minimized.

Copy link

chrisarcand commented Feb 11, 2016

Just randomly ran in to this issue yesterday. I like the proposal above 👍 Especially as I find the run_all_when_everything_filtered rather awkward.

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