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.

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

Fix spec path verification with multiple pattern #363

Merged
merged 1 commit into from
Apr 2, 2016

Conversation

thedoritos
Copy link
Contributor

The value from RSpec.configuration.pattern may not satisfy a glob pattern
if multiple pattern is set (e.g. '**{,/*/**}/*_spec.rb,**/*.feature').

And the formatter prints no spec file location warnings on STDOUT for each failed examples
because spec_path? method can't find the spec file location with a non-glob pattern.

This PR fixes spec_path? method to be able to find spec files even if a multiple pattern set.
The implementation is written referencing RSpec::Core::Configuration.

rspec-core/configuration.rb at master · rspec/rspec-core · GitHub

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 99.623% when pulling f5492bf on thedoritos:fix_spec_path_verification into 7b77566 on guard:master.

@e2
Copy link
Contributor

e2 commented Apr 2, 2016

This definitely needs a test.

Can you copy and adapt the test case from here: rspec/rspec-core@c0632ab#diff-93b32d4222d74bcb2ea7325a91ae6dc5R470 ?

I wrote the spec for the fnmatch logic above the lines you changed. And the test for those is here:

https://github.com/guard/guard-rspec/blob/master/spec/lib/guard/rspec_formatter_spec.rb#L235

@thedoritos
Copy link
Contributor Author

I agree. I have a question to implement the test.

Should I change the mock of ::RSpec.configuration to return a comma separated pattern like "**{,/*/**}/*_spec.rb,**/*_foo.rb"?
Or create a context block inside the context "when RSpec 3.0 uses ext globs" block and create another mock in it?

Thank you for the review!

@e2
Copy link
Contributor

e2 commented Apr 2, 2016

Just copy the whole context "when RSpec 3.0 uses ext globs" underneath and adapt it to reproduce the problem. Change the name of the context to match what the problem is.

I don't really understand the problem, though.

The code:

stripped = "{#{pattern.gsub(/\s*,\s*/, ',')}}"

Looks like it just removes spaces between , characters. So I'm probably missing something, or there are more issues than just one.

end

it "matches a spec file with the first pattern" do
expect(described_class.spec_path?('./spec/foo_spec.rb')).to be_truthy

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 99.625% when pulling 8b8bfc1 on thedoritos:fix_spec_path_verification into 7b77566 on guard:master.

RSpec.configuration.pattern may not satisfy a glob pattern
if multiple pattern is set (e.g. '**{,/*/**}/*_spec.rb,**/*.feature').

See also RSpec::Core::Configuration#file_glob_from.
@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 99.625% when pulling db2f2bb on thedoritos:fix_spec_path_verification into 7b77566 on guard:master.

@thedoritos
Copy link
Contributor Author

I've added the test.

The code:

stripped = "{#{pattern.gsub(/\s*,\s*/, ',')}}"

Looks like it just removes spaces between , characters.

It also wraps the pattern with parentheses { and } so that a pattern string "foo.rb,bar.rb" to be "{foo.rb,bar.rb}" and match files named foo.rb or bar.rb.

The naked pattern "foo.rb,bar.rb" is considered to be a single pattern and only matches foo.rb,bar.rb (that would be a strange file name...).

@e2 e2 merged commit 33fd884 into guard:master Apr 2, 2016
@e2
Copy link
Contributor

e2 commented Apr 2, 2016

Arigatou! :)

@e2
Copy link
Contributor

e2 commented Apr 2, 2016

Released as guard-rspec 4.6.5.

@thedoritos thedoritos deleted the fix_spec_path_verification branch April 24, 2016 03:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants