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

Raise an error if the last arg is the wrong format #96

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

ghiculescu
Copy link
Member

@ghiculescu ghiculescu commented Sep 27, 2021

I always accidentally write this:

assert_select "title", "Welcome", count: 1

When I mean to write this:

assert_select "title", text: "Welcome", count: 1

The first assertion is incorrect; it's looking for the string "Welcome", then using { count: 1 } as the assertion failure message if it's not found. So for example, if you change the text to count: 2 or count: 0 your test could behave wrong.

Since the assertion failure message probably should not be a hash, I propose we raise an error if that argument is passed. This would make it easier to identify incorrectly written tests.

I always accidentally write this:

```ruby
assert_select "title", "Welcome", count: 1
```

When I mean to write this:

```ruby
assert_select "title", text: "Welcome", count: 1
```

The first assertion is incorrect; it's looking for the string "Welcome", then using `{ count: 1}` as the assertion failure message if it's not found.

Since the assertion failure message probably should not be a hash, I propose we raise an error if that argument is passed. This would make it easier to identify incorrectly written tests.
@rafaelfranca rafaelfranca merged commit b97b9de into rails:master Nov 2, 2021
@ghiculescu ghiculescu deleted the wrong-message-arg branch November 2, 2021 02:12
kevindew added a commit to alphagov/publisher that referenced this pull request Jul 10, 2023
This test needs the second argument to be a hash, the third argument is
intended for error message. This change is required for
rails-dom-testing 2.1.0 which now raises an error if you have a
misconfigured test (it was previously silent) [1].

[1]: rails/rails-dom-testing#96
kevindew added a commit to alphagov/publisher that referenced this pull request Jul 10, 2023
This test needs the second argument to be a hash, the third argument is
intended for error message. This change is required for
rails-dom-testing 2.1.0 which now raises an error if you have a
misconfigured test (it was previously silent) [1].

[1]: rails/rails-dom-testing#96
jcowhigjr added a commit to jcowhigjr/yelp_search_demo that referenced this pull request Aug 27, 2023
…ils/rails-dom-testing#96 most likely raised a new error How: Specifying text: fixed the error
jcowhigjr added a commit to jcowhigjr/yelp_search_demo that referenced this pull request Aug 27, 2023
* Bump tailwindcss-rails from 2.0.29 to 2.0.30

Bumps [tailwindcss-rails](https://github.com/rails/tailwindcss-rails) from 2.0.29 to 2.0.30.
- [Release notes](https://github.com/rails/tailwindcss-rails/releases)
- [Changelog](https://github.com/rails/tailwindcss-rails/blob/main/CHANGELOG.md)
- [Commits](rails/tailwindcss-rails@v2.0.29...v2.0.30)

---
updated-dependencies:
- dependency-name: tailwindcss-rails
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* What: tailwindcss-rails 2.0.30 brought a dom testing gem.  This gem rails/rails-dom-testing#96 most likely raised a new error How: Specifying text: fixed the error

* rubocop fixes

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: jcowhigjr <jcowhigjr+dev@gmail.com>
Co-authored-by: John Cowhig <1895349+jcowhigjr@users.noreply.github.com>
@ccasabona
Copy link

I'm sorry I missed this change. I've written many assert_select statements as described in the problem above as such:

assert_select "title", "Welcome", count: 1

This has always seemed to work as my tests fail when the expected count of the elements is incorrect.

The implementation seems inconsistent as a test such as this one will fail if there is only one such input element on the page:

assert_select 'input#name[value=?]', 'value_of_input_element', count: 2

By setting the count to 1 or eliminating the count check altogether, the test passes.

If I go back and add the text: key, the test fails:

assert_select 'input#name[value=?]', text: 'value_of_input_element', count: 1

Nokogiri::CSS::SyntaxError: unexpected '?' after 'equal'
    nokogiri-1.15.4-x86_64 (darwin) lib/nokogiri/css/parser_extras.rb:86:in `on_error'
    racc (1.7.1) lib/racc/parser.rb:265:in `_racc_do_parse_c'
    racc (1.7.1) lib/racc/parser.rb:265:in `do_parse' 
   ...

@ghiculescu
Copy link
Member Author

@ccasabona the inconsistency is intentional. If you use the text: keyword argument, that's for matching the inner text content of the node. If you use a position argument it's for substitution.

Only this works as a shorthand:

assert_select "title", "Welcome"

But anything more complex and it's better to be explicit about what arguments you are using.

In terms of your example, I don't think you need to make any changes:

  def test_assert_select_with_question_mark
    render_html "<html><head><title>Welcome</title></head><body><input type=text value='hi' /></body></html>"

    assert_select 'input[value=?]', 'hi' # pass
    assert_select 'input[value=?]', 'hi', count: 1 # pass
    assert_select 'input[value=?]', 'hello' # fails correctly: Expected at least 1 element matching "input[value=\"hello\"]", found 0.
    assert_select 'input[value=?]', 'hello', count: 0 # pass
    assert_select 'input[value=?]', text: 'hi' # Nokogiri::CSS::SyntaxError: unexpected '?' after 'equal'
    assert_select 'input[value=?]', text: 'hi', count: 1 # Nokogiri::CSS::SyntaxError: unexpected '?' after 'equal'
end

Using text: wouldn't make sense here as you are not looking for inner text.

@ccasabona
Copy link

Thanks for making it clear about the distinction of inner text.

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

Successfully merging this pull request may close these issues.

3 participants