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

nc-annenk
Copy link
Contributor

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}/ }
Copy link
Owner

Choose a reason for hiding this comment

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

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}/ }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

else
file_or_folder
end
end.flatten.uniq
end

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

Choose a reason for hiding this comment

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

  • // 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed. Thanks for the quick feedback!

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

task :default do
sh "rspec spec/"
system "rspec spec/"
Copy link
Owner

Choose a reason for hiding this comment

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

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 = {})
Copy link
Owner

Choose a reason for hiding this comment

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

tests should not have a default value


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

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 add-exclude-pattern-option branch 4 times, most recently from 4e5b3e3 to 0dd7821 Compare February 5, 2019 17:47
@grosser grosser merged commit 4f0b5ee into grosser:master Feb 7, 2019
@nc-annenk
Copy link
Contributor Author

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

@grosser grosser mentioned this pull request Feb 7, 2019
@grosser
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
Copy link
Owner

grosser commented Feb 7, 2019

v2.28.0

@nc-annenk
Copy link
Contributor Author

nc-annenk commented Feb 7, 2019 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants