-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Add spec coverage for Admin::Trends::StatusesHelper #23898
Add spec coverage for Admin::Trends::StatusesHelper #23898
Conversation
aebc62e
to
dfe124b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
Thank you for your contribution, it's helpful overall but one of the test cases is redundant.
context 'with a status that has emoji' do | ||
let(:status) { Fabricate.build(:status, text: 'hello there :florpy:') } | ||
|
||
it 'renders a correct preview text' do | ||
result = helper.one_line_preview(status) | ||
|
||
expect(result).to eq 'hello there :florpy:' | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the :florpy:
custom emoji is never defined anywhere, that test does not really test anything different than “renders a correct preview text”. I'd suggest either removing it or defining a proper custom emoji.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch ... I feel like I had this in place and may have lost in a rebase. Will update and push changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a change which restores the creation of the florpy custom emoji.
dfe124b
to
5269572
Compare
No description provided.