Page MenuHomePhabricator

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

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
Subscribers

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested review of this revision.Dec 20 2022, 9:28 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 ...).

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
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.

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

add space where string was being broken up

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

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!