Example ids #1884

myronmarston · 2015-02-19T08:52:32Z

This introduces a new metadata key :rerun_file_path. This is usually the same as :file_path but can be different for situations such as an abstract example defined in a shared example group. In that situation :file_path would be file the shared example group is defined in (e.g. spec/support/model_shared_examples.rb), whereas :rerun_file_path would be the file would be something like spec/models/my_model_spec.rb -- a particular spec file that includes the shared example group. The :rerun_file_path is the file arg you would have to pass to rspec to cause the example to be re-run.
I wound up with a slightly different format for example ids than what was discussed in Feature idea: new --bisect flag #1767 (e.g. ./spec/rspec/core/example_spec.rb[1:14]). You can also specify multiple examples or groups: ./spec/rspec/core/example_spec.rb[1:14,1:15]. I thought the brackets make a nice syntax for separating the id part from the file part, and it makes it clear that you are passing an example id, whereas in the syntax suggested in Feature idea: new --bisect flag #1767, it's harder to differentiate between an id filter and a location filter -- file_name:1 would be ambiguous about whether it's a line number or id filter. This allows us to support passing ids simply as the main args to rspec, e.g. rspec ./spec/rspec/core/example_spec.rb[1:1] ./spec/rspec/core/example_group_spec.rb[1:1], which I think is better than a specific --ids flag. One downside to this syntax is that zsh makes you wrap it in quotes due to the use of [...]. Bash doesn't, though. I'm not sure about Windows, but it's the syntax Rake uses for task arguments so I figured it's already been vetted and proved out by rake and I have to assume rake works fine on windows.
The rerun command printed for each failed example will now use the example id if the location would match multiple examples, so that the rerun command is always guaranteed to match only the specific example it is intended for. (This is commonly needed for failed examples from shared example groups). One issue with this change: if you run zsh (as I do), you can't simply copy and paste the rerun command anymore in these situations, since zsh forces you to escape the [] characters. It'd be nice to do something about that but I'm not sure what.

TODOs:

Ensure rerun command can be copied/pasted in zsh
Add changelog entries

myronmarston · 2015-02-19T08:53:05Z

(...continued...)

What do people think about the semantics and syntax of the example id filtering?

/cc @rspec/rspec @JoshCheek

This exact spec is duplicated on line 775. I suspect this was written for the old `--line-number` CLI flag and got updated to the `file_path:line_num` form when we dropped support for `--line-number`, creating the duplication.

This allows us to uniquely identify any example or group.

myronmarston · 2015-02-23T06:35:45Z

OK, the build is green and this is ready for a review.

/cc @rspec/rspec @JoshCheek

…and add a spec enforcing that it is always up-to-date.

fables-tales · 2015-02-23T08:21:24Z

lib/rspec/core/configuration.rb

@@ -1613,6 +1613,9 @@ def extract_location(path)
          captures = match.captures
          path, lines = captures[0], captures[1][1..-1].split(":").map { |n| n.to_i }
          filter_manager.add_location path, lines
+        else
+          path, scoped_ids = path.split(/[\[\]]/)


This group is hard to read with all the slashes and square brackets, do you think it'd be easier to understand if it was path.split(/[#{Regexp.escape("[]")}]/)?

I personally find that to be less readable than what's there now, and it's almost certainly going to perform slower (it has the extra Regexp.escape method call plus the string interpolation, plus the extra objects created by "[]" and Regexp.escape...).

perhaps the regex needs a name? split(match_square_brackets) not convinced that's better, just throwing the idea out?

perhaps the regex needs a name? split(match_square_brackets) not convinced that's better, just throwing the idea out?

I'm not convinced it needs a name but am happy to assign it to a local if you find that easier to read. I could name the local on_square_brackets and then it would read split(on_square_brackets) -- do you like that?

seems fine to me :)

fables-tales · 2015-02-23T08:25:57Z

I did a partial review, I'll give more feedback when I get more time.

fables-tales · 2015-02-23T11:59:00Z

lib/rspec/core/metadata.rb

@@ -173,6 +181,12 @@ def build_description_from(parent_description=nil, my_description=nil)
          (parent_description.to_s + separator) << my_description.to_s
        end

+        def build_scoped_id_for(file_path)
+          index = @index_provider.call(file_path).to_s
+          parent_scoped_id = metadata.fetch(:scoped_id) { return index }


return keyword is unnecessary here.

It's very much necessary. It's an early return of index from build_scoped_id_for. Remember that return is relative to the enclosing method definition since this is a block, not a lambda.

fables-tales · 2015-02-23T12:01:28Z

OK, I'm done. Let me know what you think :)

JoshCheek · 2015-02-23T12:37:55Z

Today and tomorrow are pretty packed full, but I'll try and take a look sometime in the several days.

..as per @samphipppen’s request.

@samphippen

..as per @samphippen’s review request.

myronmarston · 2015-02-23T17:47:56Z

OK, I'm done. Let me know what you think :)

Thanks for the review! Any ideas what we can do about the zsh square bracket problem?

myronmarston · 2015-02-23T17:48:16Z

Today and tomorrow are pretty packed full, but I'll try and take a look sometime in the several days.

Thanks, @JoshCheek...more 👀 on a PR is always nice :).

JonRowe · 2015-02-23T21:07:34Z

I had a scan, looks good too me

myronmarston · 2015-02-23T22:49:11Z

I had a scan, looks good too me

Any ideas for what to do about zsh complaining about []? That's the main thing left here. I may just always escape ids when printing them as a re-run command but that seems kinda lame given that bash is more common...

JonRowe · 2015-02-23T22:54:13Z

I'm not a zsh user either :/

myronmarston · 2015-02-23T22:56:04Z

OK, so looks like ENV['SHELL'] tells us what shell is being used, and it's easy to look for zsh in that. We need to decide between two strategies:

Printing the example ids un-escaped by default, and only escaping them if the shell is one we know has a problem with it.
Printing the example ids escaped by default, and only disable the escaping if the shell is one we know is OK w/ [].

I'm thinking we should go with the second option. We can test the common shells (zsh, bash, csh, any others?) and for anything unknown have it escape to be on the safe side. Question is, which of these forms do we prefer?

rspec ./path/to/spec.rb\[2:2\]

# or

rspec './path/to/spec.rb[2:2]'

I think I prefer the latter...the / looks ugly.

JonRowe · 2015-02-23T22:57:01Z

I agree on the later, and on escaping unless we know we're good.

davbo · 2015-02-23T23:39:01Z

zsh users can also run the command with noglob e.g. noglob rspec ./path/to/spec.rb[2:2] lots of zsh users will already have this aliased for rake (many without knowing).

See: https://github.com/sorin-ionescu/prezto/blob/b41f4855286cd9ec2b00f7c8d91a6e2778009d51/modules/utility/init.zsh#L43
and: https://github.com/robbyrussell/oh-my-zsh/blob/cea29ff7ce4094809d0089b7469d0f9e1f22b407/plugins/rake/rake.plugin.zsh#L5

myronmarston · 2015-02-23T23:48:11Z

zsh users can also run the command with noglob e.g. noglob rspec ./path/to/spec.rb[2:2] lots of zsh users will already have this aliased for rake (many without knowing).

Right. I'm aware of that, but the re-run command should "just work" and not require users to do something special like that. Good to know, though.

- Clarify location filtering. - Add info about id filtering.

myronmarston · 2015-02-24T03:10:47Z

OK, I implemented the quoting for non-bash shells as discussed. Anyone know what other shells (if any) support the id syntax w/o quoting? I tested it on:

bash
zsh
tcsh
csh
ksh

We can add more to the list later but if anyone knows of others we can add them now.

JoshCheek · 2015-02-24T04:03:08Z

I use fish, it doesn't need quoting:

$ ruby -e 'p ARGV' ./path/to/spec.rb[2:2]
# ["./path/to/spec.rb[2:2]"]

$ fish -c 'ls [lt][im][bp]'
# ls: [lt][im][bp]: No such file or directory

$ bash -c 'ls [lt][im][bp]'
# lib:
# rspec
#
# tmp:
# aruba

@JoshCheek

…according to @JoshCheek’s comment: #1884 (comment)

myronmarston · 2015-02-24T04:12:46Z

Thanks, @JoshCheek, I added fish to the list of shells that allow unquoted ids.

JoshCheek · 2015-02-25T02:13:33Z

lib/rspec/core/example.rb

+      # Returns the location-based argument that can be passed to the `rspec` command to rerun this example.
+      def location_rerun_argument
+        @location_rerun_argument ||= begin
+          loaded_spec_files = RSpec.configuration.loaded_spec_files


I'm generally apprehensive of singletons like RSpec.configuration, and find my code is most flexible if internally I always inject such a value from the highest level of the caller. e.g. global state leads to the need to do things like https://github.com/rspec/rspec-core/blob/43b464b6bfa3117e6c91d0028d272954229f8b00/spec/support/sandboxing.rb Obviously my sense of danger tingles wherever a stateful method is called on a global object like a class, and probably it's been this way since its inception, so I'll just mention it out on this one and leave it alone.

I'm apprehensive, too, and would love to find a way to do away with the global state, but it's definitely not an easy change. Many, many call sites would have to change to accept RSpec.configuration and/or RSpec.world in order to pass them on. I looked into a bit at one point and concluded it wasn't worth the ROI, but you can certainly take a stab at it :).

myronmarston · 2015-02-25T18:11:46Z

@JonRowe / @samphippen is this good to merge?

JonRowe · 2015-02-25T21:30:23Z

LGTM

Example ids

@JoshCheek

…according to @JoshCheek’s comment: rspec#1884 (comment)

Example ids

myronmarston force-pushed the example-ids branch 4 times, most recently from e4e9bd0 to 376fc2a Compare February 23, 2015 04:44

myronmarston added 2 commits February 22, 2015 22:24

Remove duplicate spec.

67d1534

This exact spec is duplicated on line 775. I suspect this was written for the old `--line-number` CLI flag and got updated to the `file_path:line_num` form when we dropped support for `--line-number`, creating the duplication.

Add ids to examples and groups.

057e729

This allows us to uniquely identify any example or group.

myronmarston force-pushed the example-ids branch from 0b542a4 to a22d0b4 Compare February 23, 2015 06:24

myronmarston changed the title ~~WIP: Example ids~~ Example ids Feb 23, 2015

myronmarston added 3 commits February 22, 2015 22:39

Add support to filter based on example/group ids.

911601d

Use id in rerun command when the location identifies multiple examples.

ea3d536

Add some missing keys to RESERVED_KEYS.

b77d5bd

…and add a spec enforcing that it is always up-to-date.

myronmarston force-pushed the example-ids branch from a22d0b4 to b77d5bd Compare February 23, 2015 06:39

fables-tales reviewed Feb 23, 2015
View reviewed changes

myronmarston added 2 commits February 23, 2015 09:10

Name the regexp to make it more clear what it’s for.

8b71ab9

..as per @samphipppen’s request.

Standardize on setting line before rather than after.

0f7c90b

..as per @samphippen’s review request.

myronmarston force-pushed the example-ids branch from cab546a to b3f8ca5 Compare February 24, 2015 03:05

myronmarston added 3 commits February 23, 2015 19:09

Quote example ids in rerun command unless we know it’s unneeded.

3177337

Update rspec --help to include more detail about filtering.

e226831

- Clarify location filtering. - Add info about id filtering.

Add changelog entries.

16bb9a7

myronmarston force-pushed the example-ids branch from b3f8ca5 to 16bb9a7 Compare February 24, 2015 03:10

Add fish to the list of shells that allow unquoted ids.

dcbb853

…according to @JoshCheek’s comment: #1884 (comment)

JoshCheek reviewed Feb 25, 2015
View reviewed changes

JonRowe added a commit that referenced this pull request Feb 25, 2015

Merge pull request #1884 from rspec/example-ids

faeeba1

Example ids

JonRowe merged commit faeeba1 into master Feb 25, 2015

JonRowe deleted the example-ids branch February 25, 2015 21:30

myronmarston mentioned this pull request Apr 7, 2015

Bisect #1917

Merged

11 tasks

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Add fish to the list of shells that allow unquoted ids.

1f16dc8

…according to @JoshCheek’s comment: rspec#1884 (comment)

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Merge pull request rspec#1884 from rspec/example-ids

793a4b1

Example ids

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Example ids #1884

Example ids #1884

myronmarston commented Feb 19, 2015

myronmarston commented Feb 19, 2015

myronmarston commented Feb 23, 2015

fables-tales Feb 23, 2015

myronmarston Feb 23, 2015

fables-tales Feb 23, 2015

myronmarston Feb 23, 2015

fables-tales Feb 23, 2015

fables-tales commented Feb 23, 2015

fables-tales Feb 23, 2015

myronmarston Feb 23, 2015

fables-tales commented Feb 23, 2015

JoshCheek commented Feb 23, 2015

myronmarston commented Feb 23, 2015

myronmarston commented Feb 23, 2015

JonRowe commented Feb 23, 2015

myronmarston commented Feb 23, 2015

JonRowe commented Feb 23, 2015

myronmarston commented Feb 23, 2015

JonRowe commented Feb 23, 2015

davbo commented Feb 23, 2015

myronmarston commented Feb 23, 2015

myronmarston commented Feb 24, 2015

JoshCheek commented Feb 24, 2015

myronmarston commented Feb 24, 2015

JoshCheek Feb 25, 2015

myronmarston Feb 25, 2015

myronmarston commented Feb 25, 2015

JonRowe commented Feb 25, 2015

Example ids #1884

Example ids #1884

Conversation

myronmarston commented Feb 19, 2015

myronmarston commented Feb 19, 2015

myronmarston commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented Feb 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fables-tales commented Feb 23, 2015

JoshCheek commented Feb 23, 2015

myronmarston commented Feb 23, 2015

myronmarston commented Feb 23, 2015

JonRowe commented Feb 23, 2015

myronmarston commented Feb 23, 2015

JonRowe commented Feb 23, 2015

myronmarston commented Feb 23, 2015

JonRowe commented Feb 23, 2015

davbo commented Feb 23, 2015

myronmarston commented Feb 23, 2015

myronmarston commented Feb 24, 2015

JoshCheek commented Feb 24, 2015

myronmarston commented Feb 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myronmarston commented Feb 25, 2015

JonRowe commented Feb 25, 2015