Skip to content

LG-3483: Fail tests when using tag helper with deprecated class #4607

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.

Sign up for GitHub

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

Merged
aduth merged 2 commits into from
Jan 25, 2021
Merged

LG-3483: Fail tests when using tag helper with deprecated class #4607

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a4046b
LG-3483: Fail tests when using tag helper with deprecated class
aduth Jan 25, 2021
55c9fcd
Monkey-patch TagBuilder#tag_option for deprecated class detection
aduth Jan 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions spec/support/deprecated_classes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
RSpec.configure do |config|
deprecated = YAML.safe_load(File.read(File.expand_path('../../../.erb-lint.yml', __FILE__))).
dig('linters', 'DeprecatedClasses', 'rule_set').
flat_map { |rule| rule['deprecated'] }

pattern = Regexp.new "(^|\\b)(#{deprecated.join('|')})(\\b|$)"

config.before(:each) do
allow_any_instance_of(ActionView::Helpers::TagHelper::TagBuilder).
to receive(:tag_options).
with(hash_excluding(class: pattern), anything).
and_call_original
end
end

@zachmargolis zachmargolis Jan 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the use of RSpec here, it puts a lot of work on the framework. However, I think the error message is hard to parse, so I gave an alias method approach a try, WDYT?

Suggested change
RSpec.configure do |config|
deprecated = YAML.safe_load(File.read(File.expand_path('../../../.erb-lint.yml', __FILE__))).
dig('linters', 'DeprecatedClasses', 'rule_set').
flat_map { |rule| rule['deprecated'] }
pattern = Regexp.new "(^|\\b)(#{deprecated.join('|')})(\\b|$)"
config.before(:each) do
allow_any_instance_of(ActionView::Helpers::TagHelper::TagBuilder).
to receive(:tag_options).
with(hash_excluding(class: pattern), anything).
and_call_original
end
end
class ActionView::Helpers::TagHelper::TagBuilder
def self.deprecated_classes
@deprecated_classes ||= begin
YAML.safe_load(File.read(File.expand_path('../../../.erb-lint.yml', __FILE__))).
dig('linters', 'DeprecatedClasses', 'rule_set').
flat_map { |rule| rule['deprecated'] }.
map { |regex_str| Regexp.new(regex_str) }
end
end
def modified_tag_options(options, *rest)
regex = self.class.deprecated_classes.find { |r| r =~ options[:class] }
raise "CSS class '#{options[:class]}' matched regex for deprecated classes #{regex}" if regex
original_tag_options(options, *rest)
end
alias_method :original_tag_options, :tag_options
alias_method :tag_options, :modified_tag_options
end

Example error message:

  2) shared/_nav_branded.html.erb with a S3 SP-logo configured renders the logo from S3
     Failure/Error: raise "CSS class '#{options[:class]}' matched regex for deprecated classes #{regex}" if regex
     
     ActionView::Template::Error:
       CSS class 'inline-block align-middle' matched regex for deprecated classes (?-mix:align-(top|middle|bottom|baseline))

vs the current RSpec one:

  3) shared/_nav_branded.html.erb with a S3 SP-logo configured renders the logo from S3
     Failure/Error: <%= image_tag(decorated_session.sp_logo_url, height: 40,
     
       #<ActionView::Helpers::TagHelper::TagBuilder:0x00007fe3f5bdb0b0 @view_context=#<ActionView::Base:0x007fe3d8119090>> received :tag_options with unexpected arguments
         expected: (hash_not_including(:class=>/(^|\b)(align-(top|middle|bottom|baseline)|(left)-align|justify|nowrap|lin...ent)|border-(black|gray|white|aqua|orange|fuchsia|purple|maroon|darken-[1-4]|lighten-[1-4]))(\b|$)/), anything)
              got: ({:alt=>"Awesome Application!", :class=>"inline-block align-middle", :height=>40, :src=>"https://s3.us-east-1.amazonaws.com/bucket_id/key-to-logo"}, true)
       Diff:
       @@ -1,3 +1,6 @@
       -["hash_not_including(:class=>/(^|\\b)(align-(top|middle|bottom|baseline)|(left)-align|justify|nowrap|line-height-[3]|list-style-none|table(-cell)?|fit|max-width-[1-4]|mxn[4]|m[trbly]?-auto|pxn[1-4]|p[trblxy]?-auto|fixed|z[1-4]|col-(right|[579])|sm-col-11?|(md|lg)-col(-(right|[1-9][0-2]?))?|(sm|md|lg)-flex|flex-(column|none)|(items|self|justify|content)-(start|end|center|baseline|stretch)|order-([0-3]|last)|not-rounded|rounded-(top|right|bottom|left)|(md|lg)-hide|(xs|md|lg)-show|btn-(small|big|narrow|transparent)|border-(black|gray|white|aqua|orange|fuchsia|purple|maroon|darken-[1-4]|lighten-[1-4]))(\\b|$)/)",
       - "anything"]
       +[{:alt=>"Awesome Application!",
       +  :class=>"inline-block align-middle",
       +  :height=>40,
       +  :src=>"https://s3.us-east-1.amazonaws.com/bucket_id/key-to-logo"},
       + true]
       
        Please stub a default value first if message might be received with other args as well.

@aduth aduth Jan 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

However, I think the error message is hard to parse

Ah! You're right, and I'd meant to note this in the original pull request comment. I'd tinkered with this a fair bit, hoping to at least be able to use RSpec failure messages to customize the message.

An earlier, not-totally-working solution looked like this, for example:

allow_any_instance_of(ActionView::Helpers::TagHelper::TagBuilder).
  to receive(:tag_options) do |_tag_builder, options|
    expect(options).not_to include(class: pattern),
      'expected tag builder not to have been called with deprecated BassCSS class, ' +
      "got #{options[:class]}"
  end

I couldn't quite get it to work with the with variation. I thought rspec/rspec-mocks#1250 may be related, though the fix would have been included in the version we're using 🤷

Ultimately I concluded that this is only a temporary requirement as we complete the transition, so came to relent on the DevEx a bit.

But since you put together the alternative quite neatly already, I'll try that approach. I also sought not to resort to monkey-patching, though I don't have a strong reluctance against it either.

I like in your suggestion as well that it will match on individual class patterns. The only thing I notice is that we'd need to either do something similar with (^|\\b)(\\b|$), or break up the class into parts (options[:class].split(/ +/)), since otherwise it could match partial allowable classes (like USWDS' text-justify).

@zachmargolis zachmargolis Jan 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah yes I skipped over the boundary stuff since I wasn't sure if it was needed, those totally make sense, unsure if you have a preference for which direction to go

@aduth aduth Jan 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I incidentally just stumbled into the (re-)discovery of the fact that the tag helper supports class passed as an array, so it seems to make the most sense to just normalize that value as an array to test for an intersection.

@aduth aduth Jan 25, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated in 55c9fcd. I altered it a bit, patching tag_option instead, and allowing the original logic to be run. Perusing the base source, it appears the logic to build the value is non-trivial and supports more than just array, so it seemed the best way to get at the formatted value. That said, I don't love how it binds us to the specific formatting (e.g. key="value").

@zachmargolis zachmargolis Jan 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not in love either, but seems like the easiest way to catch stuff before it's rendered, changes LG!