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

add exclude pattern option #682

Merged
merged 1 commit into from Feb 7, 2019

Conversation

Projects
None yet
2 participants
@nc-annenk
Copy link
Contributor

nc-annenk commented Feb 1, 2019

Add the ability to pass --exclude-pattern

fixes #647
fixes #573

@@ -164,6 +164,7 @@ def parse_options!(argv)
BANNER
opts.on("-n [PROCESSES]", Integer, "How many processes to use, default: available CPUs") { |n| options[:count] = n }
opts.on("-p", "--pattern [PATTERN]", "run tests matching this regex pattern") { |pattern| options[:pattern] = /#{pattern}/ }
opts.on("--exclude-pattern", "--exclude-pattern [PATTERN]", "run tests matching this regex pattern") { |pattern| options[:exclude_pattern] = /#{pattern}/ }

This comment has been minimized.

@grosser

grosser Feb 1, 2019

Owner
Suggested change
opts.on("--exclude-pattern", "--exclude-pattern [PATTERN]", "run tests matching this regex pattern") { |pattern| options[:exclude_pattern] = /#{pattern}/ }
opts.on("--exclude-pattern", "--exclude-pattern [PATTERN]", "run tests not matching this regex pattern") { |pattern| options[:exclude_pattern] = /#{pattern}/ }

This comment has been minimized.

@nc-annenk

nc-annenk Feb 2, 2019

Author Contributor

Reworded.

@@ -209,16 +209,26 @@ def sort_by_filesize(tests)
end

def find_tests(tests, options = {})
tests_list = all_tests(tests, options)
exclude_tests(tests_list, options)

This comment has been minimized.

@grosser

grosser Feb 1, 2019

Owner

this seems asymmetric ... do it at the same place the pattern matching is done ?

This comment has been minimized.

@nc-annenk

nc-annenk Feb 2, 2019

Author Contributor

Addressed.

else
file_or_folder
end
end.flatten.uniq
end

def exclude_tests(tests_list, options = {})
tests_list.reject { |file| file =~ (options[:exclude_pattern] || //) }

This comment has been minimized.

@grosser

grosser Feb 1, 2019

Owner
  • // matches everything
  • use either reject! or grep since they are more efficient
  • do as little work as possible in the inner loop (use local variable and not option lookup)
Suggested change
tests_list.reject { |file| file =~ (options[:exclude_pattern] || //) }
if regex = options[:exclude_pattern]
tests_list -= tests_list.grep(regex)
end

This comment has been minimized.

@nc-annenk

nc-annenk Feb 2, 2019

Author Contributor

Addressed. Thanks for the quick feedback!

@nc-annenk nc-annenk force-pushed the nc-annenk:add-exclude-pattern-option branch from 2ef59df to f57719e Feb 2, 2019

Rakefile Outdated
@@ -3,5 +3,5 @@ require 'bump/tasks'
require 'bundler/gem_tasks'

task :default do
sh "rspec spec/"
system "rspec spec/"

This comment has been minimized.

@grosser

grosser Feb 2, 2019

Owner

This need to fail when the command fails, so use sh

@@ -208,15 +208,22 @@ def sort_by_filesize(tests)
tests.map! { |test| [test, File.stat(test).size] }
end

def find_tests(tests, options = {})
(tests || []).map do |file_or_folder|
def find_tests(tests = [], options = {})

This comment has been minimized.

@grosser

grosser Feb 2, 2019

Owner

tests should not have a default value


if regex = options[:exclude_pattern]
files_list.reject! { |file| file =~ regex }
end

This comment has been minimized.

@grosser

grosser Feb 2, 2019

Owner

this is not consistent with how pattern works, it should be done inside so it does not apply to folders
... or both should apply to folders 🤷‍♂️

... and while your in here: it should use .flat_map + [file_or_folder] and not .flatten and .uniq!

This comment has been minimized.

@nc-annenk

nc-annenk Feb 4, 2019

Author Contributor

My assumption would be that anything matching the regex for the include pattern would be included, and the exclude_pattern will filter out anything matching the exclude pattern. It should apply to folders or files, as the exclude pattern behavior should mirror the behavior of native rspec --exclude-pattern option.

@nc-annenk nc-annenk force-pushed the nc-annenk:add-exclude-pattern-option branch 4 times, most recently from 4e5b3e3 to 0dd7821 Feb 4, 2019

@nc-annenk nc-annenk force-pushed the nc-annenk:add-exclude-pattern-option branch from 0dd7821 to 4826f07 Feb 7, 2019

@grosser grosser merged commit 4f0b5ee into grosser:master Feb 7, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nc-annenk

This comment has been minimized.

Copy link
Contributor Author

nc-annenk commented Feb 7, 2019

@grosser Thanks for merging! Is there anything I need to do to get a new gem version out?

@grosser grosser referenced this pull request Feb 7, 2019

Merged

cleanup pattern matching #683

@grosser

This comment has been minimized.

Copy link
Owner

grosser commented Feb 7, 2019

no, I'm just waiting for my cleanup PR to go green and will then release

@grosser

This comment has been minimized.

Copy link
Owner

grosser commented Feb 7, 2019

v2.28.0

@nc-annenk

This comment has been minimized.

Copy link
Contributor Author

nc-annenk commented Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.