Relax expectations for flash messages in tests #16889
Draft
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
From https://docs.rubocop.org/rubocop-capybara/cops_capybara.html#capybaraspecificactions: > This cop is deprecated. We plan to remove this in the next major > version update to 3.0.
To make the test code more intentional when we want to be precise with flash messages, and less verbose when we just want to dismiss them, the flash helpers do not have implicit values for `:type` and `:message` anymore, and do not require them to be present either. Previously `expect_flash` default value for `type` was `:success`, meaning that code like `expect_flash(message: "An error occurred")` implicitly expected a `:success` flash, which is not obvious by reading the code. This commit changes the default value for `type` to `:any` for all flash helpers, so that there is no implicit expectations on the flash type anymore. Additionally, it also relaxes the expectation on the message text so that `expect_and_dismiss_flash` and other helpers can be called without `:type` or `:message` parameters, and it will be done regardless of the flash type and message text. Finally it also adds documentation to all flash helpers.
|
I'm not sure why you would want to expect a flash, but not it's type. This should be part of the test? |
Good question. I think I remember some times where I added an expectation on the flash for synchronization, and I had to indicate the type but that was not my purpose. I just wanted to discard any flash. But maybe my memories are wrong and that's not useful and your initial PR should be preferred. |
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.
Ticket
None
Follow up to #16789 (this comment) and #16863.
What are you trying to accomplish?
Make the test code more intentional when we want to be precise with flash messages, and less verbose when we just want to dismiss them, the flash helpers do not have implicit values for
:typeand:messageanymore, and do not require them to be present either.What approach did you choose and why?
Previously
expect_flashdefault value fortypewas:success, meaning that code likeexpect_flash(message: "An error occurred")implicitly expected a:successflash, which is not obvious by reading the code.This commit changes the default value for
typeto:anyfor all flash helpers, so that there is no implicit expectations on the flash type anymore.expect_flash(message: "An error occurred")would then check the flash message text and only the flash message text, which is a fair expectation.Additionally, it also relaxes the expectation on the message text so that
expect_and_dismiss_flashand other helpers can be called without:typeor:messageparameters, and it will be done regardless of the flash type and message text.Finally it also adds documentation to all flash helpers.
Merge checklist