Update spec/views/notifications/index.html.erb_spec.rb file #5924
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
…tifications factorybots
guswhitten
requested review from
compwron,
FireLemons,
littleforest and
elasticspoon
as code owners
elasticspoon
requested changes
Jul 19, 2024
There was a problem hiding this comment.
Thanks for this! The tests are largely good but if you don't mind could you change the RSpec expectations to use hard coded values instead of values from the factories.
It makes the tests easier to read and instills more confidence in the tests. See my comments for specifics.
Comment on lines
45
to
47
| expect(notifications_html[0].text).to include(notification_1_hour_ago.event.message) | ||
| expect(notifications_html[1].text).to include(notification_1_day_ago.event.message) | ||
| expect(notifications_html[2].text).to include(notification_2_days_ago.event.message) |
There was a problem hiding this comment.
Can we hard code these? expect(notifications_html[0].text).to include("Message 0")
| patch_note_index = notifications_html.index { |node| node.text.include?("Patch Notes") } | ||
|
|
||
| expect(patch_note_index).to eq(3) | ||
| expect(notifications_html[patch_note_index + 1].text).to include(notification_3_days_ago.event.message) |
There was a problem hiding this comment.
There are a few other places in this code where it does something like: expect(page).to include(patch_note_a.note) could you also change those to use hard coded values?
github-actions
bot
added
the
dependencies
guswhitten
added a commit
to guswhitten/casa
that referenced
this pull request
Aug 7, 2024
elasticspoon
pushed a commit
that referenced
this pull request
Aug 14, 2024
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.
What github issue is this PR for, if any?
Resolves #5685
Resolves #5686
Resolves #5687
What changed, and why?
Update and add tests to file
spec/views/notifications/index.html.erb_spec.rbusing new FactoryBot notificationsHow is this tested? (please write tests!) 💖💪
This PR is only tests
Screenshots please :)
Feelings gif (optional)