Fix more bugs in declarative specs filter #49141
Conversation
ghiculescu
force-pushed
the
declarative-specs-2
branch
from
1b2ba92
to
160bbd1
Compare
|
Red tests are due to rails/globalid#168 |
| filter = filter.gsub(/\s+/, '[\s_]+') | ||
| filter = filter.gsub("\\ ", '[\s_]+').gsub(/\s+/, '[\s_]+') |
There was a problem hiding this comment.
Is this fixing a bug, or is this just expanding the set of regexps we accept? If the latter, when is it necessary to write a filter like that?
I am hesitant to do any more transformations here. I can accept gsub(/\s+/, '[\s_]+') as a necessary evil, but manipulating regexps is very error prone. Consider the following:
Regexp.new("[\\ ]")
# => /[\ ]/
Regexp.new("[\\ ]".gsub("\\ ", '[\s_]+'))
# => /[[\s_]+]/
Regexp.new("[\\ ]".gsub("\\ ", '[\s_]+')).match?("+")
# => true
Regexp.new("\\\\ ")
# => /\\ /
Regexp.new("\\\\ ".gsub("\\ ", '[\s_]+'))
# warning: regular expression has ']' without escape: /\[\s_]+/
# => /\[\s_]+/
Regexp.new("\\\\ ".gsub("\\ ", '[\s_]+')).match?("[ _]")
# warning: regular expression has ']' without escape: /\[\s_]+/
# => trueBasically, we have to draw the line somewhere.
There was a problem hiding this comment.
I do a lot of my test running using the m gem. The regex I'm testing here comes from there. Specifically from the Regexp#escape call on this line.
I realise it's not part of Rails so if the answer is we can't support it, I can try and find a way to support this in m. It just feels like it's something we should support since it's not like that gem is doing something really funky with the regex.
There was a problem hiding this comment.
I've pushed a commit that I think solves the problem. Instead of wading through the weeds of regexps, we can simply join the original filter and the underscore-normalized filter.
Also, I discovered why the filter was being escaped over and over as you described in #47942 (comment): the same options instance is used for each call to Rails::LineFiltering#run, and we were modifying it in place each time. I've fixed that, so escape_declarative_test_filter (now renamed normalize_declarative_test_filter) no longer has to be idempotent.
Let me know what you think.
There was a problem hiding this comment.
Instead of wading through the weeds of regexps, we can simply join the original filter and the underscore-normalized filter.
Damn. That's a great idea.
There was a problem hiding this comment.
This approach looks great to me 💯
Thanks for your help!!
| @@ -5,7 +5,7 @@ | |||
| module Rails | |||
| module LineFiltering # :nodoc: | |||
| def run(reporter, options = {}) | |||
| options[:filter] = Rails::TestUnit::Runner.compose_filter(self, options[:filter]) | |||
| options = options.merge(filter: Rails::TestUnit::Runner.compose_filter(self, options[:filter])) | |||
There was a problem hiding this comment.
You could use options.merge! here @jonathanhefner
There was a problem hiding this comment.
That would defeat the purpose. 😄 See #49141 (comment).
Follow-up to rails#47942. This commit simplifies the normalization of regexp test filters. Instead of modifying a given regexp to match a union of whitespace and underscores, the regexp is unioned with the underscore-normalized version of itself. This allows filters that include escaped spaces, such as `/foo\ bar/`. This commit also fixes `Rails::LineFiltering#run` such that normalization isn't reapplied for every test suite. Thus `normalize_declarative_test_filter` is no longer required to be idempotent. Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
jonathanhefner
force-pushed
the
declarative-specs-2
branch
from
2309347
to
0a8e537
Compare
|
Thank you, @ghiculescu! 👓 |
ref: #47942
This PR fixes some things that were missed in first PR.
Minitest::Specdoesn't extendRails::LineFiltering, socompose_filter(and thusescape_declarative_test_filter) were not being called in the tests we added. This PR adds a new test case withRails::LineFilteringincluded to confirm behaviour is the same either way.escape_declarative_test_filterdidn't work on regexes that had passed throughRegexp#escape. Now it supports that too.