Suggested by @palys-swm:
Also, this is a great function to have some tests.
https://phab.comm.dev/D4317#121940
Took like a minute. Not sure if it helps much, but it doesn't hurt?
Details
Details
Ran tests
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| lib/utils/text-utils.js | ||
|---|---|---|
| 7 ↗ | (On Diff #13679) | Without this change, we got nonsense output when maxNumberOfNouns was 0: pluralize(['cat', 'dog', 'sheep'], 0) was returning: Received: "cat, dog and 4 others" |
Comment Actions
Best for a separate diff, but wondering if there a good way we can:
- Add this test to CI, and
- Run this test in lint-staged if the relevant file is changed?
Comment Actions
Add this test to CI
yarn workspace lib test is already included in the CI
Run this test in lint-staged if the relevant file is changed?
Will look at adding unit tests to pre-commit hooks
Comment Actions
- Cool, might be a good thing to create a task for that
- lint-staged should be fast, so we should probably avoid globally running jest for the whole project (unless that's fast / there's a way to make it fast)
Comment Actions
Tests look good but there's some redundancy that can be removed.
| lib/utils/text-utils.js | ||
|---|---|---|
| 7 ↗ | (On Diff #13679) | Good call on this. If maxNumberOfNouns === 0, I can't think of a reason to return anything but an empty string. |
| lib/utils/text-utils.test.js | ||
| 10 ↗ | (On Diff #13679) | This test covers the case when nouns.length === 3 and maxNumberOfNouns === 3 (because of the default parameter). This case is repeated on line 13: expect(pluralize(['cat', 'dog', 'sheep'], 3)).toBe('cat, dog, and sheep');Not sure if you included both of these to test the default parameter vs. the explicit parameter, but in my opinion the tests are equivalent since we know the behavior of default parameters in JavaScript. Additionally, since the expected output is the same with the only difference being the values in nouns, only one test is needed since nouns will always be an array of strings. |
| 11 ↗ | (On Diff #13679) | This tests the case when nouns.length === 4 and maxNumberOfNouns === 3 (default parameter, again). This case is repeated in the test on lines 18-20: expect(pluralize(['cat', 'dog', 'sheep', 'moose'])).toBe( 'cat, dog, and 2 others', ); I think you only need one of these tests by the same reasoning above. |
Comment Actions
Not sure if you included both of these to test the default parameter vs. the explicit parameter,
Yeah that's why I included them. The redundancy was intentional
Comment Actions
Oh, okay my bad! I only made those suggestions since if this is part of the CI build we want it to run as fast as possible so we should eliminate redundant tests.
Comment Actions
No worries, I think that for tests we should err on the side of covering more cases vs eliminating redundancy.
As for CI build times, the cost of an additional unit test is orders of magnitude too small to make a difference
Comment Actions
Thanks for creating these tests!
I think they can be improved by using describe / it. We would have a single test suite for text utils, then a separate ones for each function, and then a separate test for each case. Something like this:
describe('Text utils", () => {
describe('pluralize function', () => {
it('should return a single word when a single word is given, () => {
expect(pluralize(['a'])).toBe('a');
};
...
};
...
};There are a couple of advantages:
- We can clearly see which tests pass - instead of seeing only the first failed test
- An intention of a test is explained
- Tests are grouped together into logical sections
On the other hand creating these would take more time, but it should be worth it if we plan to analyze test reports.
What do you think?
Comment Actions
@palys-swm thanks for the suggestion, this looks a lot better
and each test is "self-documenting"