Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Example ids #1884

Merged
merged 11 commits into from
Feb 25, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Lint/LiteralInInterpolation:

# This should go down over time.
MethodLength:
Max: 155
Max: 40

# Exclude the default spec_helper to make it easier to uncomment out
# default settings (for both users and the Cucumber suite).
Expand Down
5 changes: 5 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ Enhancements:
`RSpec::Core::Reporter#publish(event_name, hash_of_attributes)`. (Jon Rowe, #1869)
* Remove dependency on the standard library `Set` and replace with `RSpec::Core::Set`.
(Jon Rowe, #1870)
* Assign a unique id to each example and group so that they can be
uniquely identified, even for shared examples (and similar situations)
where the location isn't unique. (Myron Marston, #1884)
* Use the example id in the rerun command printed for failed examples
when the location is not unique. (Myron Marston, #1884)

Bug Fixes:

Expand Down
6 changes: 6 additions & 0 deletions lib/rspec/core/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1606,13 +1606,19 @@ def absolute_pattern?(pattern)
end
end

# @private
ON_SQUARE_BRACKETS = /[\[\]]/

def extract_location(path)
match = /^(.*?)((?:\:\d+)+)$/.match(path)

if match
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(ON_SQUARE_BRACKETS)
filter_manager.add_ids(path, scoped_ids.split(/\s*,\s*/)) if scoped_ids
end

return [] if path == default_path
Expand Down
31 changes: 25 additions & 6 deletions lib/rspec/core/example.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,32 @@ def inspect_output
inspect_output
end

# Returns the argument that can be passed to the `rspec` command to rerun this example.
def rerun_argument
loaded_spec_files = RSpec.configuration.loaded_spec_files
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :).


Metadata.ascending(metadata) do |meta|
return meta[:location] if loaded_spec_files.include?(meta[:absolute_file_path])
Metadata.ascending(metadata) do |meta|
return meta[:location] if loaded_spec_files.include?(meta[:absolute_file_path])
end
end
end

# Returns the location-based argument that can be passed to the `rspec` command to rerun this example.
#
# @deprecated Use {#location_rerun_argument} instead.
# @note If there are multiple examples identified by this location, they will use {#id}
# to rerun instead, but this method will still return the location (that's why it is deprecated!).
def rerun_argument
location_rerun_argument
end

# @return [String] the unique id of this example. Pass
# this at the command line to re-run this exact example.
def id
Metadata.id_from(metadata)
end

# @attr_reader
#
# Returns the first exception raised in the context of running this
Expand Down Expand Up @@ -138,7 +155,9 @@ def initialize(example_group_class, description, user_metadata, example_block=ni
@example_block = example_block

@metadata = Metadata::ExampleHash.create(
@example_group_class.metadata, user_metadata, description, example_block
@example_group_class.metadata, user_metadata,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always think multiple comma separated args on the same line is a smell, if one of them changes it'll churn the line with both of them making it less obvious in the diff what happened, how about

@metadata = Metadata::ExampleHash.create(
  @example_group_class.metadata, 
  user_metadata,
  example_group_class.method(:next_runnable_index_for),
  description,
  example_block,
)

Note the trailing comma, which means that if we add another argument, that line won't churn.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always think multiple comma separated args on the same line is a smell, if one of them changes it'll churn the line with both of them making it less obvious in the diff what happened

So every time you call a method with multiple arguments you break the arguments up into separate lines? I certainly haven't noticed that in your PRs....

In general, I don't think it's a code smell to put multiple args on the same line. We do it all over the place in RSpec and in every other ruby code base I've seen.

Regardless, while I like splitting a long arg list across a few lines to avoid a super long line, I dislike going as far as one-arg-per-line because of the extra vertical space it takes up. That in turn limits how much code I can look at on my screen at one time. I like the 3 line form I've used here because it avoids the single super long line, looks pretty decent visually (IMO) and avoids begin overly vertically long.

Note the trailing comma, which means that if we add another argument, that line won't churn.

I like doing that, but 1.8.7 can't parse it so we can't adopt it for RSpec until we drop 1.8.7 support :(.

example_group_class.method(:next_runnable_index_for),
description, example_block
)

@example_group_instance = @exception = nil
Expand Down
19 changes: 18 additions & 1 deletion lib/rspec/core/example_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,9 @@ def self.set_it_up(description, *args, &example_group_block)
user_metadata = Metadata.build_hash_from(args)

@metadata = Metadata::ExampleGroupHash.create(
superclass_metadata, user_metadata, description, *args, &example_group_block
superclass_metadata, user_metadata,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same thing as above inre commas and args?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same response :).

superclass.method(:next_runnable_index_for),
description, *args, &example_group_block
)

hooks.register_globals(self, RSpec.configuration.hooks)
Expand Down Expand Up @@ -414,6 +416,15 @@ def self.children
@children ||= []
end

# @private
def self.next_runnable_index_for(file)
if self == ExampleGroup
RSpec.world.num_example_groups_defined_in(file)
else
children.count + examples.count
end + 1
end

# @private
def self.descendants
@_descendants ||= [self] + FlatMap.flat_map(children, &:descendants)
Expand Down Expand Up @@ -573,6 +584,12 @@ def self.declaration_line_numbers
FlatMap.flat_map(children, &:declaration_line_numbers)
end

# @return [String] the unique id of this example group. Pass
# this at the command line to re-run this exact example group.
def self.id
Metadata.id_from(metadata)
end

# @private
def self.top_level_description
parent_groups.last.description
Expand Down
25 changes: 20 additions & 5 deletions lib/rspec/core/filter_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,15 @@ def add_location(file_path, line_numbers)
# locations is a hash of expanded paths to arrays of line
# numbers to match against. e.g.
# { "path/to/file.rb" => [37, 42] }
locations = inclusions.delete(:locations) || Hash.new { |h, k| h[k] = [] }
locations[File.expand_path(file_path)].push(*line_numbers)
inclusions.add(:locations => locations)
add_path_to_arrays_filter(:locations, File.expand_path(file_path), line_numbers)
end

def add_ids(rerun_path, scoped_ids)
# ids is a hash of relative paths to arrays of ids
# to match against. e.g.
# { "./path/to/file.rb" => ["1:1", "2:4"] }
rerun_path = Metadata.relative_path(File.expand_path rerun_path)
add_path_to_arrays_filter(:ids, rerun_path, scoped_ids)
end

def empty?
Expand All @@ -32,7 +38,9 @@ def prune(examples)
examples.select { |e| include?(e) }
else
locations = inclusions.fetch(:locations) { Hash.new([]) }
examples.select { |e| priority_include?(e, locations) || (!exclude?(e) && include?(e)) }
ids = inclusions.fetch(:ids) { Hash.new([]) }

examples.select { |e| priority_include?(e, ids, locations) || (!exclude?(e) && include?(e)) }
end
end

Expand Down Expand Up @@ -62,6 +70,12 @@ def include_with_low_priority(*args)

private

def add_path_to_arrays_filter(filter_key, path, values)
filter = inclusions.delete(filter_key) || Hash.new { |h, k| h[k] = [] }
filter[path].concat(values)
inclusions.add(filter_key => filter)
end

def exclude?(example)
exclusions.include_example?(example)
end
Expand All @@ -82,7 +96,8 @@ def prune_conditionally_filtered_examples(examples)
# and there is a `:slow => true` exclusion filter), but only for specs
# defined in the same file as the location filters. Excluded specs in
# other files should still be excluded.
def priority_include?(example, locations)
def priority_include?(example, ids, locations)
return true if MetadataFilter.filter_applies?(:ids, ids, example.metadata)
return false if locations[example.metadata[:absolute_file_path]].empty?
MetadataFilter.filter_applies?(:locations, locations, example.metadata)
end
Expand Down
31 changes: 25 additions & 6 deletions lib/rspec/core/metadata.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,21 @@ def self.backtrace_from(block)
[block.source_location.join(':')]
end

# @private
def self.id_from(metadata)
"#{metadata[:rerun_file_path]}[#{metadata[:scoped_id]}]"
end

# @private
# Used internally to populate metadata hashes with computed keys
# managed by RSpec.
class HashPopulator
attr_reader :metadata, :user_metadata, :description_args, :block

def initialize(metadata, user_metadata, description_args, block)
def initialize(metadata, user_metadata, index_provider, description_args, block)
@metadata = metadata
@user_metadata = user_metadata
@index_provider = index_provider
@description_args = description_args
@block = block
end
Expand Down Expand Up @@ -151,6 +157,8 @@ def populate_location_attributes
metadata[:line_number] = line_number.to_i
metadata[:location] = "#{relative_file_path}:#{line_number}"
metadata[:absolute_file_path] = File.expand_path(relative_file_path)
metadata[:rerun_file_path] ||= relative_file_path
metadata[:scoped_id] = build_scoped_id_for(relative_file_path)
end

def file_path_and_line_number_from(backtrace)
Expand All @@ -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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return keyword is unnecessary here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"#{parent_scoped_id}:#{index}"
end

def ensure_valid_user_keys
RESERVED_KEYS.each do |key|
next unless user_metadata.key?(key)
Expand All @@ -196,7 +210,7 @@ def ensure_valid_user_keys

# @private
class ExampleHash < HashPopulator
def self.create(group_metadata, user_metadata, description, block)
def self.create(group_metadata, user_metadata, index, description, block)
example_metadata = group_metadata.dup
group_metadata = Hash.new(&ExampleGroupHash.backwards_compatibility_default_proc do |hash|
hash[:parent_example_group]
Expand All @@ -208,7 +222,7 @@ def self.create(group_metadata, user_metadata, description, block)
example_metadata.delete(:parent_example_group)

description_args = description.nil? ? [] : [description]
hash = new(example_metadata, user_metadata, description_args, block)
hash = new(example_metadata, user_metadata, index, description_args, block)
hash.populate
hash.metadata
end
Expand All @@ -229,15 +243,15 @@ def full_description

# @private
class ExampleGroupHash < HashPopulator
def self.create(parent_group_metadata, user_metadata, *args, &block)
def self.create(parent_group_metadata, user_metadata, example_group_index, *args, &block)
group_metadata = hash_with_backwards_compatibility_default_proc

if parent_group_metadata
group_metadata.update(parent_group_metadata)
group_metadata[:parent_example_group] = parent_group_metadata
end

hash = new(group_metadata, user_metadata, args, block)
hash = new(group_metadata, user_metadata, example_group_index, args, block)
hash.populate
hash.metadata
end
Expand Down Expand Up @@ -308,15 +322,20 @@ def full_description
# @private
RESERVED_KEYS = [
:description,
:description_args,
:described_class,
:example_group,
:parent_example_group,
:execution_result,
:file_path,
:absolute_file_path,
:rerun_file_path,
:full_description,
:line_number,
:location,
:block
:scoped_id,
:block,
:shared_group_inclusion_backtrace
]
end

Expand Down
9 changes: 9 additions & 0 deletions lib/rspec/core/metadata_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def filter_applies?(key, value, metadata)
silence_metadata_example_group_deprecations do
return filter_applies_to_any_value?(key, value, metadata) if Array === metadata[key] && !(Proc === value)
return location_filter_applies?(value, metadata) if key == :locations
return id_filter_applies?(value, metadata) if key == :ids
return filters_apply?(key, value, metadata) if Hash === value

return false unless metadata.key?(key)
Expand All @@ -42,6 +43,14 @@ def filter_applies_to_any_value?(key, value, metadata)
metadata[key].any? { |v| filter_applies?(key, v, key => value) }
end

def id_filter_applies?(rerun_paths_to_scoped_ids, metadata)
scoped_ids = rerun_paths_to_scoped_ids.fetch(metadata[:rerun_file_path]) { return false }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return keyword is also unnecessary here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also necessary here, for the same reasons -- I'm using return to early exit from id_filter_applies? with a value of false. Clearly scoped_ids = false doesn't make sense.


Metadata.ascend(metadata).any? do |meta|
scoped_ids.include?(meta[:scoped_id])
end
end

def location_filter_applies?(locations, metadata)
line_numbers = example_group_declaration_lines(locations, metadata)
line_numbers.empty? || line_number_filter_applies?(line_numbers, metadata)
Expand Down
50 changes: 49 additions & 1 deletion lib/rspec/core/notifications.rb
Original file line number Diff line number Diff line change
Expand Up @@ -485,7 +485,7 @@ def colorized_totals_line(colorizer=::RSpec::Core::Formatters::ConsoleCodes)
def colorized_rerun_commands(colorizer=::RSpec::Core::Formatters::ConsoleCodes)
"\nFailed examples:\n\n" +
failed_examples.map do |example|
colorizer.wrap("rspec #{example.rerun_argument}", RSpec.configuration.failure_color) + " " +
colorizer.wrap("rspec #{rerun_argument_for(example)}", RSpec.configuration.failure_color) + " " +
colorizer.wrap("# #{example.full_description}", RSpec.configuration.detail_color)
end.join("\n")
end
Expand Down Expand Up @@ -515,6 +515,54 @@ def fully_formatted(colorizer=::RSpec::Core::Formatters::ConsoleCodes)

formatted
end

private

def rerun_argument_for(example)
location = example.location_rerun_argument
return location unless duplicate_rerun_locations.include?(location)
conditionally_quote(example.id)
end

def duplicate_rerun_locations
@duplicate_rerun_locations ||= begin
locations = RSpec.world.all_examples.map(&:location_rerun_argument)

Set.new.tap do |s|
locations.group_by { |l| l }.each do |l, ls|
s << l if ls.count > 1
end
end
end
end

# Known shells that require quoting: zsh, csh, tcsh.
#
# Feel free to add other shells to this list that are known to
# allow `rspec ./some_spec.rb[1:1]` syntax without quoting the id.
#
# @private
SHELLS_ALLOWING_UNQUOTED_IDS = %w[ bash ksh fish ]

def conditionally_quote(id)
return id if shell_allows_unquoted_ids?
"'#{id.gsub("'", "\\\\'")}'"
end

def shell_allows_unquoted_ids?
return @shell_allows_unquoted_ids if defined?(@shell_allows_unquoted_ids)

@shell_allows_unquoted_ids = SHELLS_ALLOWING_UNQUOTED_IDS.include?(
# Note: ENV['SHELL'] isn't necessarily the shell the user is currently running.
# According to http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html:
# "This variable shall represent a pathname of the user's preferred command language interpreter."
#
# It's the best we can easily do, though. We err on the side of safety (quoting
# the id when not actually needed) so it's not a big deal if the user is actually
# using a different shell.
ENV['SHELL'].to_s.split('/').last
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I wind up using this a lot, would be nice to be able to set it in my ~/.rspec, b/c my $SHELL is set to /bin/bash, even though I use /usr/local/bin/fish. I think it was because so many tools assume bash that it was breaking things, but that was a while ago, so don't remember exactly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I wind up using this a lot, would be nice to be able to set it in my ~/.rspec

Set what, specifically? What shell RSpec thinks you are using? Or whether or not the ids are quoted?

Also, you said fish, like bash allows the unquoted square bracket id syntax so it seems like it will work OK even though your $SHELL isn't actually what you use, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it will work. I checked it, and fish behaves correctly either way, so it probably doesn't actually affect me. When I said it, I was thinking it'd be nice to say --shell /usr/local/bin/fish, but I guess it's more of a question of whether anything else is likely to use this variable. If not, then specifying quote preference is going to be more correct and more obvious as to what its purpose is.

)
end
end

# The `ProfileNotification` holds information about the results of running a
Expand Down
Loading