Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prevent Aliased Matchers from Overriding Expected Data #1116

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Prevent Aliased Matchers from Overriding Expected Data #1116

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4bc09e8
Prevent aliased matchers from overriding data
May 24, 2019
bee5647
Give option to override aliased matcher name no matter the position
May 31, 2019
7e54f1f
Remove lambda default parameter to maintain ruby compatability
May 31, 2019
16b836b
Split aliased matcher description into new class
May 31, 2019
514ba9e
Remove keyword arguments for backwards compatibility
May 31, 2019
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 lib/rspec/matchers/dsl.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ module DSL
# @see RSpec::Matchers
def alias_matcher(new_name, old_name, options={}, &description_override)
description_override ||= lambda do |old_desc|
old_desc.gsub(EnglishPhrasing.split_words(old_name), EnglishPhrasing.split_words(new_name))
old_desc.sub(EnglishPhrasing.split_words(old_name), EnglishPhrasing.split_words(new_name))
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to only match the start of the string, this might still accidentally override terms if the original description is not generated.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! Can you clarify this a bit? #sub matches the first occurrence, so are you saying something like matching the first n characters of the string?

Also, what do you mean by if the original description is not generated? Since BaseMatcher implements description, I would think that they all would have a description. Unless a class implements its own description which does not contain the name of the matcher.

Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify this a bit? #sub matches the first occurrence, so are you saying something like matching the first n characters of the string?

"hello_old_name".sub("old_name","new_name") would produce "hello_new_name" but we don't want that. We only want to replace old_name as it was generated initially.

Also, what do you mean by if the original description is not generated? Since BaseMatcher implements description, I would think that they all would have a description.

Similarly it is possible for matchers to implement description themselves and it might mean they don't contain the old_name at all, in this case we shouldn't be replacing the first instance blindly as it might not be in the description but in the expected.

end
klass = options.fetch(:klass) { AliasedMatcher }

Expand Down
13 changes: 13 additions & 0 deletions spec/rspec/matchers/aliased_matcher_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,19 @@ def description
expect(matcher.description).to eq("my blockless override description")
end

RSpec::Matchers.define :my_repeating_base_matcher do
def description
"my repeating base matcher my repeating base matcher"
end
end

RSpec::Matchers.alias_matcher :my_repeating_override, :my_repeating_base_matcher

it 'does not override data in the description with the same name as the matcher' do
matcher = my_repeating_override
expect(matcher.description).to eq("my repeating override my repeating base matcher")
Copy link
Member

Choose a reason for hiding this comment

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

It'd prefer a practical example showing the problem using fail_with, even the one from rspec/rspec-rails#1835 would suffice.

end

it 'works properly with a chained method off a negated matcher' do
expect { }.to avoid_outputting.to_stdout

Expand Down