Fix broken spec assertions #4484
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `assert_no_selector` method is a method from the Capybara DSL that only has an effect if you are navigating to a URL in the test and populating a page attribute. In these tests there are is no page attribute and thus no selectors to match against (just an empty string). I verified these tests did nothing by checking them against `assert_no_selector "*"` and they passed that. As these are assertions that refute a state, these were likely not noticed as these assertions would only fail if we visited a page that had the selector. I chose the approach of using `assert_select "selector", false` as this seemed to be the most common refute selector in this repo with ~180 of them compared to ~25 of `assert_select "selector", count: 0`. I used the `count: 0` syntax when passing arguments other hash arguments as this felt more idiomatic. Annoyingly a new `assert_no_select` method will be available in the next `rails-dom-testing` gem release (which provides `assert_select`) which would be a better option than both of the above. [1] [1]: https://github.com/rails/rails-dom-testing/blob/da0d96bc872e7e9b2656bc17ef3467bdf4bd9d48/CHANGELOG.md#next--unreleased
The previous commit fixed that the `assert_no_selector` method was not actually doing any assertions. This revealed that this test was actually failing as the rendering was producing the HTML that we were trying to assert was absent. To fix this I've had to modify the content item to have the links that indicate it is part of a step-by-step nav.
The previous commits contain fixes for when Capybara assertions were mixed into tests that didnt actually make use of the Capybara DSL and thus were not actually asserting correctly. To try prevent this happening again I've configured the Capybara helpers (and thus assertions) to only be available to tests that are tagged with `:capybara`. This will mean that trying to use `assert_no_selector` will raise a NoMethodError rather than having a test that passes with whatever is given. Ideally we'd not have had to put a tag of `capybara` on and instead group this just to specs for features (those in /specs/features) however in this test suite there isn't a clear test pyramid distinction and the browser feature specs are frequently mixed in with more regular unit tests. This meant taking the more clunky tag approach was needed. I've also fixed tests that were broken due to reliance on Capybara assert methods.
It doesn't strike me that they'll ever be a scenario where we want visual regression tests without capybara. Therefore I've added the visual_regression tag as one to also use the Capybara::DSL. While doing this I took the liberty to update the visual_regression tag usage to be the short form.
andysellick
approved these changes
Dec 10, 2024
There was a problem hiding this comment.
Seems sensible to me, great job noticing and fixing this @kevindew 👏
Might be worth having a BE pair of eyes on this as well though.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR was prompted after discovering in #4451 that the commonly used
assert_no_selectortest assertion in view specs didn't actually assert anything and would pass even if givenassert_no_selector("*")The reason this non-assertion was available was because it is using a Capybara assertion (from the Capybara::DSL) - however these assertions only have an impact if you have run
visit(url)and have apagevariable.To remedy this I have taken 2 steps:
assert_no_selector(selector)to beassert_select(selector, false)to confirm when a selector isn't available for a view spec (unfortunatelyassert_no_selectisn't available yet)capybaratag so that only specs that are tagged with capybara will have access to the Capybara DSL - to prevent using assertions that have no impact. Ideally we'd only have these scoped to a type of test (i.e. feature) but the ship has sailed on that for this project.No changelog as this is only an internal change.