[no-Jira] Improve test coverage #851
Conversation
There was a problem hiding this comment.
For the most part this looks really good and adds some good code coverage, thanks!
Some things to note:
-
You asked about commits, some things you could have done, which may have been slightly nicer, would have been to put the commit "Remove redundant GET request" after the test commits (TDD principle). You could also have combined the cleanup commits for
eventForm.jsinto a single commit and have the description be a bulleted list of the cleanup actions done. -
There are a few places where the test sentencing is a bit weird, I believe you did this to avoid putting a
describeblock around a singleitblock but retain the function name. There are probably trade-offs to readability and useless boilerplate here. I'm ok either way on it, but it does seem a tad jarring to read. -
There are quite a few places (new and old) where tests are referencing hard-coded UUIDs and text. We should really work towards removing this habit from the codebase and use the reference locations of these values instead. Otherwise, if we change a value in the
testData.spec.jsfile we'll have to search and replace it all over the codebase instead of it just working properly.
canac
force-pushed
the
test-coverage
branch
2 times, most recently
from
e60d16d
to
396c0eb
Compare
* Simplify allowGroupRegistration calculation * Use an object for regValidate map * Document non-existent properties * Merge identical functions * Simplify pageIsVisible check
* Remove unused dependency * Remove redundant statement * Improve makePositionArray readability
|
@wrandall22 Do you want to take another look, or should I dismiss your review? |
Increase the test coverage of some of the most frequently-modified files.