refactor(input): switch to fakeAsync tests #8490
Conversation
googlebot
added
the
cla: yes
src/lib/input/input.spec.ts
Outdated
| @@ -160,7 +160,7 @@ describe('MatInput without forms', function () { | |||
| expect(el.classList.contains('mat-form-field-empty')).toBe(true); | |||
| }); | |||
|
|
|||
| it('should not be empty after input entered', async(() => { | |||
| it('should not be empty after input entered', () => { | |||
There was a problem hiding this comment.
Should we make these fakeAsync as well? if they do async things in their constructor or lifecycle hooks they could silently leak async operations. I like that fakeAsync fails if you don't flush it - no more time wasted binary searching for the leaky test
There was a problem hiding this comment.
I think they were async to begin with because we just had one async and then we copy-pasted it when we wrote new tests, but they don't really need to be. As for those silent error leaks, I believe they only happen with the async zone which we won't use anymore.
There was a problem hiding this comment.
I see, so what happens if we don't mark something fakeAsync but it actually does do async stuff? Even if they're not currently async, I'm worried about introducing code that causes them to become async in the future, forgetting to update the tests, and then later running into some obscure failure related to it
There was a problem hiding this comment.
I'm not a fan of having to add fakeAsync even though we might not need it. Presumably if we add some that is async in the future, we'd know what PR introduced it and we'd fix any failures before they make it into master.
There was a problem hiding this comment.
That's not really the way its worked out in the past. There have been a number of times where our CI was red due to async test failures and nobody really knew how to track down. If we preemptively make everything fakeAsync then its really obvious which test has the issue.
crisbeto
force-pushed
the
input-fake-async
branch
from
02463d0 to
5bfb87c
Compare
|
I've switched everything over to |
mmalerba
added
pr: lgtm
action: merge
crisbeto
force-pushed
the
input-fake-async
branch
from
5bfb87c to
0442f88
Compare
Moves all of the input tests away from the `async` zone either to the default one or to `fakeAsync`.
crisbeto
force-pushed
the
input-fake-async
branch
from
0442f88 to
742394a
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Moves all of the input tests away from the
asynczone either to the default one or tofakeAsync.