Unit test suite for InlineColorEditor #2174
Conversation
|
Oops, not quite ready yet...forgot to add the unit tests for the text field. |
|
OK, should be ready for review now. |
|
Reviewing |
|
|
||
| // The hRatio1/vRatio1 are for setting up the pre-drag conditions. The expected value | ||
| // should be related to the hRatio2 or vRatio2. | ||
| function testDrag(itemMember, hRatio1, vRatio1, hRatio2, vRatio2, param, expectedValue, tolerance) { |
There was a problem hiding this comment.
One thing I was considering doing (since this is kind of "parameter hell") was changing these args to be an object (with fields named after the args), so the calls would be easier to read. The downside is that it makes the calls a lot more verbose, but maybe that's worth it for legibility. What do you think?
There was a problem hiding this comment.
Being verbose wouldn't hurt here, especially given the number of params.
There was a problem hiding this comment.
Done. Hopefully it will be clearer now (though a lot longer :))
|
Nicely done, especially without firing up a new window. I guess this stuff doesn't rely on ProjectManager, so that's a bonus. Initial review complete. |
|
Changes pushed. I also added JSDoc comments to all the helper functions, since some of their args were just non-obvious enough to require some explanation. |
|
Just added one last little refactoring cleanup I noticed. |
|
Looks good. Merging. |
Unit test suite for InlineColorEditor
@jasonsanjose
A few of these unit tests got checked in as part of an earlier pull request (and were reviewed by Peter F), but some of those got modified in this pull anyway, so it's probably worth just reviewing everything from scratch.
Note that these are all mocked tests in one form or another--the first set use a full InlineColorEditor and a mock editor/document (so the InlineColorEditor doesn't actually live in the mock editor); the later ones just test ColorEditor by itself (not inside an InlineColorEditor).
Let me know if you think I should build some more integration-y tests as well. I think these tests give pretty good overall coverage though.