Page MenuHomePhabricator
Differential D5953

[lib] introduce unit tests for stringForReactionList reaction util function
ClosedPublic
Actions

Authored by ginsu on Dec 20 2022, 9:16 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 5:16 PM
Unknown Object (File)
Fri, Nov 8, 5:15 PM
Unknown Object (File)
Fri, Nov 8, 5:15 PM
Unknown Object (File)
Fri, Nov 8, 5:15 PM
Unknown Object (File)
Fri, Nov 8, 5:15 PM
Unknown Object (File)
Fri, Nov 8, 5:15 PM
Unknown Object (File)
Fri, Nov 8, 4:51 PM
Unknown Object (File)
Tue, Nov 5, 7:29 AM
View All Files
Subscribers
ashoat
atul
tomek

Details

Reviewers
atul
tomek
Commits
rCOMM6527cd38b18c: [lib] introduce unit tests for stringForReactionList reaction util function
Summary

introduced unit tests for stringForReactionList reaction util function. This was suggested by Atul in D5878. In these tests I tested the case where there are multiple reactions of the same reaction type, mutliple and single reaction of different reactions, no reactions, and a single reaction for one reaction type, as well as when the viewer has reacted.

Depends on D5878

Test Plan

Ran yarn test reaction-utils.test.js` in lib directory and all the tests pass as expected

Screenshot 2022-12-20 at 12.12.29 PM.png (516×2 px, 464 KB)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.Dec 20 2022, 9:16 AM
Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptDec 20 2022, 9:16 AM
ginsu edited the summary of this revision. (Show Details)Dec 20 2022, 9:18 AM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, tomek.
ginsu added a parent revision: D5878: [lib] introduce stringForReactionList in reaction-utils.
ginsu attached a referenced file: F295320: Screenshot 2022-12-20 at 12.12.29 PM.png. (Show Details)
ginsu mentioned this in D5878: [lib] introduce stringForReactionList in reaction-utils.
Harbormaster completed remote builds in B14524: Diff 19781.Dec 20 2022, 9:28 AM
ginsu requested review of this revision.Dec 20 2022, 9:28 AM
tomek accepted this revision.Dec 20 2022, 9:48 AM
tomek added inline comments.
lib/shared/reaction-utils.test.js
6–7 ↗(On Diff #19781)

We should try to make the descriptions read as sentences. So in this case, the sentence starts with it, so it should be something like it('should return ...).

ginsu updated this revision to Diff 19794.Dec 20 2022, 10:39 AM

address tomek's feedback

Harbormaster completed remote builds in B14537: Diff 19794.Dec 20 2022, 10:50 AM
atul accepted this revision.Dec 20 2022, 11:33 AM

Looks good, thanks for writing these out!

Might be good to consider some edge cases though.

For example when there's an emoji with more than 9 reacts (eg 10 or 40) do we want to display 10 and 40 or 9+? Maybe if there are more than 99 reacts we'd want to do 99+?

We should also make sure the function behaves correctly when there are no reactions at all (we probably wouldn't call the function in that situation, but just to make sure it's airtight and there's no unexpected behavior)

This revision is now accepted and ready to land.Dec 20 2022, 11:33 AM
atul added inline comments.Dec 20 2022, 11:35 AM
lib/shared/reaction-utils.test.js
69 ↗(On Diff #19794)

These lines extend beyond 80 characters which is what we'd expect and what Prettier/ESLint give us. However, might be good to break the string up using + so they it gets formatted onto multiple lines to appease the team's character width preferences.

ginsu updated this revision to Diff 19955.Dec 21 2022, 8:00 AM

address atul's comments

ginsu added a comment.Dec 21 2022, 8:07 AM
In D5953#178859, @atul wrote:

For example when there's an emoji with more than 9 reacts (eg 10 or 40) do we want to display 10 and 40 or 9+? Maybe if there are more than 99 reacts we'd want to do 99+?

Good idea, I need to modify stringForReactionList to handle this case. Will write a follow-up diff with the changes in stringForReactionList and the test

We should also make sure the function behaves correctly when there are no reactions at all (we probably wouldn't call the function in that situation, but just to make sure it's airtight and there's no unexpected behavior)

The fifth unit test (lines 82-86) should cover this

Harbormaster completed remote builds in B14632: Diff 19955.Dec 21 2022, 8:14 AM
ginsu updated this revision to Diff 19956.Dec 21 2022, 8:30 AM

add space where string was being broken up

Harbormaster completed remote builds in B14633: Diff 19956.Dec 21 2022, 8:44 AM
ginsu added a child revision: D5974: [lib] modified stringForReactionList to handle more than 9 likes/reactions for the same reaction type.Dec 21 2022, 8:46 AM
ginsu added a comment.Dec 21 2022, 8:48 AM

Good idea, I need to modify stringForReactionList to handle this case. Will write a follow-up diff with the changes in stringForReactionList and the test

D5974

atul added a comment.Dec 21 2022, 10:01 AM

The fifth unit test (lines 82-86) should cover this

Ah missed that, thanks for pointing it out.

In D5953#179582, @ginsu wrote:

Good idea, I need to modify stringForReactionList to handle this case. Will write a follow-up diff with the changes in stringForReactionList and the test

D5974

Sweet, thanks for putting that up!

Closed by commit rCOMM6527cd38b18c: [lib] introduce unit tests for stringForReactionList reaction util function (authored by ginsu). · Explain WhyDec 21 2022, 10:57 AM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMM6527cd38b18c: [lib] introduce unit tests for stringForReactionList reaction util function.

Revision Contents
Changeset List

PathSize
lib/
shared/
114 lines

Diff 19956

View Options

lib/shared/reaction-utils.test.js

Loading...