Refactor assertString: Faster, less nested and more consistent. #2372
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2372 +/- ##
==========================================
- Coverage 99.95% 99.95% -0.01%
==========================================
Files 107 107
Lines 2454 2445 -9
Branches 619 617 -2
==========================================
- Hits 2453 2444 -9
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
Hi, @WikiRik!
|
|
codecov/project will be fixed with #2341 |
test/util.test.js
Outdated
| }); | ||
|
|
||
| it('Should not throw an error if the argument is a string', () => { | ||
| assert.doesNotThrow(() => { assertString('testing'); }); |
There was a problem hiding this comment.
Can we keep the empty string unit test? Then we will not forget that we accept that on purpose
There was a problem hiding this comment.
Yes, we can. I'll create and commit it.
There was a problem hiding this comment.
Now we've this unit teste:
There was a problem hiding this comment.
while we are at it, we should likely also test cases for null and undefined.
Maybe NaN too? And I guess true and false should also be tested against, for full coverage of all types :-)
There was a problem hiding this comment.
I created the null input validation and also added the suggested tests. I tried to add the following test:
it('Should not throw an error if the argument is a String instance', () => { assert.doesNotThrow(() => { assertString(new String('antidisestablishmentarianism')); }); });
But some ESLint rules prevent this approach, ESLint "no-new-wrappers.
WikiRik
requested review from
profnandaa,
tux-tn,
rubiin,
ezkemboi and
pano9000
|
when you pass literally because it is not caught by the first check. Should we explicitly check for null there as well? |
src/lib/util/assertString.js
Outdated
|
|
||
| throw new TypeError(`Expected a string but received a ${invalidType}`); | ||
| } | ||
| if (input === undefined) throw new TypeError(`Expected a string but received a ${input}`); |
There was a problem hiding this comment.
should probably check for null as well, see #2372 (comment)
There was a problem hiding this comment.
I will add null validation and create tests for both cases.
test/util.test.js
Outdated
| }); | ||
|
|
||
| it('Should not throw an error if the argument is a string', () => { | ||
| assert.doesNotThrow(() => { assertString('testing'); }); |
There was a problem hiding this comment.
while we are at it, we should likely also test cases for null and undefined.
Maybe NaN too? And I guess true and false should also be tested against, for full coverage of all types :-)
|
Hello, @pano9000! |
@profnandaa,
I have a possible improvement for the assertString.
I am reducing the number of necessary validations and also a more assertive approach, in my point of view (if this approach doesn't make sense, I would appreciate feedback on it, if possible).
The performance has also improved, it wasn't something incredible, but it was considerable.
Unit tests:
