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

Add --exclude_pattern option to configuration #1651

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jmondo
Contributor

jmondo commented Jul 28, 2014

Resolves #1626. It allows passing --exclude_pattern to rspec on the command line.

Afaik we still can't use the power of rake tasks. This is because the rake task still uses FileList to include files. And once files are included, they can't be changed by passing --pattern or --exclude-pattern (there is a warning about this when you try). Fortunately, once #1589 is merged, it will be possible to pass --exclude-pattern within rspec_opts in the rake task config. This is because the rake task's pattern option will use --pattern which can be combined with --exclude-pattern for a one-two punch.

@myronmarston Let me know how this looks! Sorry it took so long.

When I run `rspec`
Then the output should contain "3 examples, 0 failures"
Scenario: The `--exclude-pattern` flag makes RSpec first match the default pattern, then skip files matching the specified exclude pattern

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

The wording on this ("...makes RSpec first match the default pattern...") makes it sound like --exclude-pattern can't be combined with --pattern. If that's true, we should find a way to make them work together. If it's not true, we should find a better wording for this :).

This comment has been minimized.

@jmondo

jmondo Jul 28, 2014

Contributor

Ah you're right. They do work together. I'll work on that wording.

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

Also, maybe we can add a scenario showing them working together?

This comment has been minimized.

@jmondo

jmondo Jul 28, 2014

Contributor

Added.

@@ -1,7 +1,7 @@
Feature: pattern
Use the `--pattern` option to tell RSpec to look for specs in files that match
a pattern other than `"**/*_spec.rb"`.
a pattern instead of the default `"**/*_spec.rb"`.

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

👍 This reads better, thanks.

@@ -193,6 +193,19 @@ def pattern=(value)
@pattern = value
end
# @macro define_reader
# Load files except those matching this pattern.

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

This wording suggests RSpec will load every file on the file system that does not match the specified pattern. Maybe this is better?

# Excludes files that match the specified pattern from being loaded by RSpec.

This comment has been minimized.

@jmondo

jmondo Jul 28, 2014

Contributor

I think that's clearer. Won't match the wording of pattern though. Is that ok? Pattern:

# Load files matching this pattern (default: `'**/*_spec.rb'`))

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

It would be nice to use similar warning but I care more about accuracy and clarity. Any ideas for a consistent wording that is clear and accurate for both?

This comment has been minimized.

@jmondo

jmondo Jul 28, 2014

Contributor

"Load files matching this pattern" (existing)
"Exclude files matching this pattern" might work.

This comment has been minimized.

@jmondo

jmondo Jul 28, 2014

Contributor

Changed it to this, let me know if you would prefer something else. Seems succinct and not misleading.

When I run `rspec --exclude-pattern "**/{models,features}/*_spec.rb"`
Then the output should contain "0 examples, 0 failures"
Scenario: The `--exclude-pattern` flag can be used with the `--pattern` flag

This comment has been minimized.

@jmondo

jmondo Jul 28, 2014

Contributor

The new test!

assign_files_or_directories_to_run file
expect(config.files_to_run).to include(file)
end

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

Why did you remove this spec?

This comment has been minimized.

@jmondo

jmondo Jul 28, 2014

Contributor

Ah right meant to comment about that. It wasn't testing anything. Take a look at https://github.com/jmondo/rspec-core/blob/exclude-pattern/lib/rspec/core/configuration.rb#L1289 . When you run rspec with a file specified (not a directory), it loads that file rather than doing the pattern matching. Try changing "/resources/a_foo.rb" in the test on 562 to one of the other specs in the resources folder and it should still pass.

This comment has been minimized.

@cupakromer

cupakromer Jul 28, 2014

Member

I'll agree this is a slightly poor spec. Though, it is testing something. It's just not explicitly clear from the spec name. This is likely a duplicate of the spec on line 408. The name of spec 408 is clearer, however, it never sets a pattern, so it's arguably incomplete - unless it was intended to by-pass the setting of a default pattern?

Once we sort out if these are dupes, or should be testing slightly different things, I think it's possible to improve the description to clear up any confusion.

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

It is a weird spec, but I think there's value in having a spec that shows what happens when users pass a specific file name (rather than a directory), and how that relates to pattern. It looks like the behavior is that the pattern is ignored and the file is loaded since it was explicitly specified. I think it would be useful to replace this with a spec that calls assign_files_or_directories_to_run with two file names -- one that matches the pattern and one that doesn't -- and have it show that both files are included in files_to_run, demonstrating that the pattern is not applied to specific file names.

This makes me realize we should probably also spec how exclude_pattern relates to specific files...seems like it is most consistent to do the same thing: specific files are always loaded regardless of whether or not they match either pattern or exclude_pattern.

Does that make sense?

This comment has been minimized.

@jmondo

jmondo Jul 30, 2014

Contributor

Yep. Working on it now.

end
context "after files have already been loaded" do
it 'will warn that it will have no effect' do

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

Not a merge blocker, but I'd prefer it 'warns....' rather than it 'will warn...'. It's more inline with our phrasing and is more terse while being just as clear.

config.exclude_pattern = "rspec/**/*.spec"
end
it 'will not warn if reset is called after load_spec_files' do

This comment has been minimized.

@myronmarston

myronmarston Jul 28, 2014

Member

Same here.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jul 31, 2014

BTW, in my review of #1589 I found some bugs in the --pattern option. #1652 has fixes. I think your new --exclude-pattern is prone to the same bugs so it would be good to fix the same issues for your option as part of this PR.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jul 31, 2014

I merged #1653 (which was basically #1589 rebased and fixed up) so can you add --exclude-pattern to the rake task as well?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jul 31, 2014

BTW, if there reaches a point where you don't have time or desire to continue working on this, let us know and we'll be happy to pick it up. We like to let contributors finish there PRs when they are able, though :).

@jmondo

This comment has been minimized.

Contributor

jmondo commented Jul 31, 2014

Good to know. I'm planning on finishing it! Let me know if I need to hurry up.

@jmondo

This comment has been minimized.

Contributor

jmondo commented Jul 31, 2014

Actually feel free to make the changes required to support those other two merges. Let me know if there's anything I should do with my feature after that.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Aug 1, 2014

OK. I've got a couple other PRs on my list to tackle first, but I'll add yours to my list. In the meantime, if you want to take this further, feel free :).

@myronmarston

This comment has been minimized.

Member

myronmarston commented Aug 18, 2014

I'm going to pick this up in the next couple days and get it ready to merge.

myronmarston added a commit that referenced this pull request Aug 20, 2014

@myronmarston

This comment has been minimized.

Member

myronmarston commented Aug 20, 2014

Closing in favor of #1671.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment