There's only three functions in text-utils so might as well crank out some unit tests for all of them.
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 | ||
|---|---|---|
| 28–30 ↗ | (On Diff #13680) | Before, a call to trimText('', 0)/trimText('', 1)/etc would return "..." which exceeds the maxLength argument. Going forward, we won't include ellipses for strings that are longer than maxLength and \leq three characters. This ensures that what's returned by trimText is strictly within maxLength. |
Comment Actions
Looks good! Appreciate the explanation for erring on the side of redundancy for unit tests, I wholeheartedly agree with that. With that in mind, these tests look good!
| lib/utils/text-utils.js | ||
|---|---|---|
| 27–30 ↗ | (On Diff #13681) | Kinda sucks there's no good way to refactor these cases where information isn't "lost" when the ellipsis gets added. Using your tests on lines 69 and 70 as an example: expect(trimText('the quick brown fox jumps', 3)).toBe('the');
expect(trimText('the quick brown fox jumps', 4)).toBe('t...');As the maxLength grows, you'd expect to be able to read more information from the trimmed text, i.e., when maxLength === 4, you'd expect to read more of the string than when maxLength === 3, because 4 > 3. However, when maxLength === 3, you can read 'the', but when maxLength === 4, you can only read 't'. It's all good though! I don't think there's a good solution that works with the ellipsis characters being considered as part of the maxLength. |
Comment Actions
Kinda sucks there's no good way to refactor these cases where information isn't "lost" when the ellipsis gets added. Using your tests on lines 69 and 70 as an example:
Yeah, I didn't know what the best approach here was.
eg when text = "hello" and maxLength = 2
- We could display an ellipsis, but that's three characters and exceeds maxLength
- We could display ".." but that's unclear and doesn't reflect anything about the original text
- We could display "he" but then it's not clear that there's more that follows and was cut off
- We could display nothing?
Comment Actions
Yup, that is confusing. Luckily, just did a quick check throughout the codebase and there are no calls to trimText with a maxLength argument of less than 30. So for now, this should be fine.
| lib/utils/text-utils.js | ||
|---|---|---|
| 27–30 ↗ | (On Diff #13681) | I don't think this is a big issue - I don't expect this function to be used with such a short limit. |
| lib/utils/text-utils.test.js | ||
| 41–42 ↗ | (On Diff #13681) | If we expect the text to be a its length is less or equal to 4 - the second assertion is redundant. |
| 70–73 ↗ | (On Diff #13681) | Maybe add a case with something between these, e.g. 10 |
| lib/utils/text-utils.test.js | ||
|---|---|---|
| 41–42 ↗ | (On Diff #13681) | The toBeLessThanOrEqual tests were about ensuring that the length of what is returned is <= maxLength. It is redundant, but it was supposed to hold constant another variable. Will modify this file to use the same describe( it( style as pluralize and maybe that'll be more clear since there's room to describe the test |
| lib/utils/text-utils.test.js | ||
|---|---|---|
| 86 | It looks like Prettier isn't functioning here? (The line is longer than 80 chars) | |