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

Fix some 6-1-stable tests for Ruby 2.5 and 2.6 #45074

Merged
merged 1 commit into from May 12, 2022

Conversation

p8
Copy link
Member

@p8 p8 commented May 11, 2022

Summary

6-1-stable currently has failing tests: https://buildkite.com/rails/rails/builds/86353

  assert_equal "<the-name #{escaped_dangerous_chars}=\"the value\"></the-name>",
    tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value")

  TagHelperTest#test_tag_builder_with_dangerous_unknown_attribute_name [/rails/actionview/test/template/tag_helper_test.rb:179]:
  --- expected
  +++ actual
  @@ -1 +1 @@
  -"<the-name _______________=\"the value\"></the-name>"
  +"<the-name>{&quot;&amp;&lt;&gt;\\&quot;&#39; %*+,/;=^|&quot;=&gt;&quot;the value&quot;}</the-name>"

The test fails because the attributes hash argument has string keys.
In Ruby 2.5 and 2.6 only Symbol keys are allowed in keyword arguments.
So the argument is seen as the content argument for the tag instead of the
attributes.

This test was introduced in 123f42a.
Running the test prior to 123f42a generates the same error.
Calling to_sym on the key fixes the test.

A simpler solution would be skipping this test altogether for Ruby 2.7, but
then we might miss future regressions.

@rails-bot rails-bot bot added the actionview label May 11, 2022
@p8 p8 changed the title Ignore test for Ruby < 2.7 Fix 6-1-stable build May 11, 2022
@p8 p8 changed the title Fix 6-1-stable build Fix 6-1-stable build for Ruby 2.5 and 2.6 May 11, 2022
@p8 p8 changed the title Fix 6-1-stable build for Ruby 2.5 and 2.6 Fix some 6-1-stable tests for Ruby 2.5 and 2.6 May 11, 2022
tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value", escape: false)
if RUBY_VERSION >= "2.7"
assert_equal "<the-name #{escaped_dangerous_chars}=\"the value\"></the-name>",
tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value")
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried:

Suggested change
tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value")
tag.public_send(:"the-name", { COMMON_DANGEROUS_CHARS => "the value" })

Pretty sure that will work across versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

That still fails:

def tag_string(name, content = nil, **options, &block)
  puts "#{name}, #{content}, #{options.inspect}"
end

tag_string('a-name', "abc" => "def")
# a-name, {"abc"=>"def"}, {}
tag_string('a-name', {"abc" => "def"})
# a-name, {"abc"=>"def"}, {}
tag_string('a-name', :abc  => "def")
# a-name, , {:abc=>"def"}

I've changed the PR to call to_sym on the key instead.

6-1-stable currently has failing tests: https://buildkite.com/rails/rails/builds/86353

      assert_equal "<the-name #{escaped_dangerous_chars}=\"the value\"></the-name>",
        tag.public_send(:"the-name", COMMON_DANGEROUS_CHARS => "the value")

      TagHelperTest#test_tag_builder_with_dangerous_unknown_attribute_name [/rails/actionview/test/template/tag_helper_test.rb:179]:
      --- expected
      +++ actual
      @@ -1 +1 @@
      -"<the-name _______________=\"the value\"></the-name>"
      +"<the-name>{&quot;&amp;&lt;&gt;\\&quot;&rails#39; %*+,/;=^|&quot;=&gt;&quot;the value&quot;}</the-name>"

The test fails because the `attributes` hash argument has string keys.
In Ruby 2.5 and 2.6 only Symbol keys are allowed in keyword arguments.
So the argument is seen as the `content` argument for the tag instead of the
`attributes`.

This test was introduced in 123f42a.
Running the test prior to 123f42a generates the same error.

Calling `to_sym` on the key fixes the test.
@p8 p8 force-pushed the ci/fix-6-1-stable-build branch from 7735a94 to 507b5aa Compare May 12, 2022 08:01
@byroot
Copy link
Member

byroot commented May 12, 2022

You're right, after digging into this myself, I realized that hash is passed to **options, so yes it must be symbols.

@byroot byroot merged commit 1672a71 into rails:6-1-stable May 12, 2022
@p8 p8 deleted the ci/fix-6-1-stable-build branch May 12, 2022 13:48
@p8
Copy link
Member Author

p8 commented May 12, 2022

Thanks! I think this should be backported to 6-0-stable as well:
https://buildkite.com/rails/rails/builds/86406#adfc3986-f771-429e-9666-059b546ecbaa

@p8
Copy link
Member Author

p8 commented May 12, 2022

Should I create a PR?

@byroot
Copy link
Member

byroot commented May 12, 2022

Nah don't worry, I'll cherry-pick the commit.

eileencodes pushed a commit that referenced this pull request Jul 12, 2022
Fix some 6-1-stable tests for Ruby 2.5 and 2.6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants