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

[aduth]

Why: As a developer, I expect that RSpec automated render testing will prevent me from adding deprecated classes, so that I'm not working against the effort to transition from BassCSS to USWDS.

Implemented using RSpec mock expecting any occurrence of tag builder to not be invoked using a class option including one of the classes configured in ERBLint as deprecated (see #4599).

Testing Instructions:

Try to add a deprecated class to tag helper in view covered by RSpec tests.

Example:

diff --git a/app/views/shared/_nav_branded.html.erb b/app/views/shared/_nav_branded.html.erb
index 2e124797a..448b0fe74 100644
--- a/app/views/shared/_nav_branded.html.erb
+++ b/app/views/shared/_nav_branded.html.erb
@@ -5,5 +5,5 @@
     <span class='absolute top-0 bottom-0 border-right my1 sm-my2'></span>
   </div>
   <%= image_tag(decorated_session.sp_logo_url, height: 40,
-                alt: decorated_session.sp_name, class: 'inline-block text-middle') %>
+                alt: decorated_session.sp_name, class: 'inline-block align-middle') %>
 </nav>

[zachmargolis]

LGTM, added a suggestion for error readability

[zachmargolis]

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]

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]

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]

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]

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]

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