test(): Add cursor animation testing and migrate some easy one to jest #9829
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
Build Stats
|
….js into cursor-animation-testing
asturur
changed the title
….js into cursor-animation-testing
asturur
changed the title
| this.abortCursorAnimation(); | ||
| this._currentCursorOpacity = 1; |
There was a problem hiding this comment.
this is done by abortCursorAnimation
There was a problem hiding this comment.
Most if not all the comments are code styling differences. So disregard them if they don't suit you.
However, I do think a test should mock stuff using jest API instead of assignment so the test is clear, easy to track and strict in terms of side effects and cleanup.
| const iText = new IText('hello\nhello'); | ||
| iText._tick = _tickMock; | ||
| iText.enterEditing(); | ||
| expect(iText._tick).toHaveBeenCalledWith(); |
There was a problem hiding this comment.
Should be
expect(tickMock)
There was a problem hiding this comment.
I think the opposite, i did wrong at doing expect(_initDelayedMock) in the other test.
| iText.enterEditing(); | ||
| expect(iText._tick).toHaveBeenCalledWith(); | ||
| _tickMock.mockClear(); | ||
| iText.__lastSelected = true; |
There was a problem hiding this comment.
I don't think setting internal state makes a robust test
There was a problem hiding this comment.
it is how it was before, otherwise i have to go with a full canvas where i click and select, i just moved the old tests as much as possible
| iText.enterEditing(); | ||
| // enter editing will set the cursor and set selection to 1 | ||
| expect(selection).toBe(1); | ||
| expect(iText._tick).toBeCalledWith(); |
There was a problem hiding this comment.
I would declare the mock as a const and expect that to be called
| @@ -0,0 +1,348 @@ | |||
| /* eslint-disable @typescript-eslint/no-non-null-assertion */ | |||
There was a problem hiding this comment.
Many tests here are the same apart from params and can become
test.each
There was a problem hiding this comment.
no i really like the tests one after the other is on level of nesting less, they are never exactly the same.
You end up having parameters like selectionStart, selectionEnd, direction, expected value and you end up writing the test in the parameters instead of the test function.
….js into cursor-animation-testing
Description
Adding cursor animation testing with jest fake timers.
This PR migrates some qunit test to jest since it was easier to do so than just move the timing part to jest.
The tests are the same as before with the exception of 'assertCursorAnimation' that has been replaced with a check of the call of 'initDelayedCursor' method of IText.
This because in the PR #9823 a lot of test fail by changing how the animation fire.
The current tests assert if an animation state is on or off, but they do not actually assert who did that and if the animation was a previous running one.
With the check on the initDelayedCursor method we have a more reliable way to know if the last action started an animation without having to depend on timings
We also can snapshot the animation itself as an array of values to see how the animation changes