-
-
Notifications
You must be signed in to change notification settings - Fork 753
Example ids #1884
Example ids #1884
Changes from all commits
67d1534
057e729
911601d
ea3d536
b77d5bd
8b71ab9
0f7c90b
3177337
e226831
16bb9a7
dcbb853
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
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 | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Note the trailing comma, which means that if we add another argument, that line won't churn. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same thing as above inre commas and args? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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) | ||
|
@@ -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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) | ||
|
@@ -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 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return keyword is unnecessary here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's very much necessary. It's an early return of |
||
"#{parent_scoped_id}:#{index}" | ||
end | ||
|
||
def ensure_valid_user_keys | ||
RESERVED_KEYS.each do |key| | ||
next unless user_metadata.key?(key) | ||
|
@@ -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] | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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 | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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 } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return keyword is also unnecessary here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's also necessary here, for the same reasons -- I'm using |
||
|
||
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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Set what, specifically? What shell RSpec thinks you are using? Or whether or not the ids are quoted? Also, you said There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
) | ||
end | ||
end | ||
|
||
# The `ProfileNotification` holds information about the results of running a | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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/orRSpec.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 :).