Skip to content

feat(zone.js): Add 'flush' parameter to fakeAsync to flush after the test#57137

Closed
atscott wants to merge 1 commit intoangular:mainfrom
atscott:fakeAsyncFlush
Closed

feat(zone.js): Add 'flush' parameter to fakeAsync to flush after the test#57137
atscott wants to merge 1 commit intoangular:mainfrom
atscott:fakeAsyncFlush

Conversation

@atscott
Copy link
Copy Markdown
Contributor

@atscott atscott commented Jul 25, 2024

From the internal issue on the matter:

When using the standard Jasmine version of it promises returned by the body function are automatically awaited. The Catalyst version of it is fake-async, so awaiting the promise does not make sense; however it would be nice if Catalyst automatically flushed the promise to replicate the experience of using standard it. This would allow users to do the following:

it('should fail later', async () => {
  await new Promise(r => setTimeout(r));
  fail('failure');
});

In Catalyst today the above test will pass. If this proposal to automatically flush the resulting promise were implemented it would fail.

Flushing after the tests complete has been the default behavior inside
Google since 2020. Very few tests remain that use the old behavior of
only flushing microtasks. The example above would actually fail with
fakeAsync due to the pending timer, but the argument still remains the
same. We might as well just flush if we're going to fail the test
anyways by throwing if there's no flush at the end.

@atscott atscott added the target: minor This PR is targeted for the next minor release label Jul 25, 2024
@pullapprove pullapprove Bot requested review from JiaLiPassion July 25, 2024 19:45
@angular-robot angular-robot Bot added detected: feature PR contains a feature commit area: core Issues related to the framework runtime labels Jul 25, 2024
@ngbot ngbot Bot modified the milestone: Backlog Jul 25, 2024
@atscott atscott force-pushed the fakeAsyncFlush branch 2 times, most recently from 292dd78 to aaf028c Compare July 25, 2024 19:51
@atscott atscott added target: patch This PR is targeted for the next patch release action: global presubmit The PR is in need of a google3 global presubmit and removed target: minor This PR is targeted for the next minor release labels Jul 25, 2024
@angular-robot angular-robot Bot added the detected: breaking change PR contains a commit with a breaking change label Jul 25, 2024
@atscott atscott force-pushed the fakeAsyncFlush branch 6 times, most recently from c3b517f to 433a68f Compare July 26, 2024 16:02
@atscott
Copy link
Copy Markdown
Contributor Author

atscott commented Jul 26, 2024

TGP has 1 failure, trivially fixed by using flush: false

@atscott atscott requested a review from alxhub July 26, 2024 17:42
@atscott atscott removed the action: global presubmit The PR is in need of a google3 global presubmit label Aug 1, 2024
@atscott
Copy link
Copy Markdown
Contributor Author

atscott commented Aug 2, 2024

TGP is green with the previously failing test being fixed ahead of time.

@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Aug 2, 2024
@thePunderWoman thePunderWoman added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Aug 2, 2024
@angular-robot angular-robot Bot removed the detected: breaking change PR contains a commit with a breaking change label Aug 2, 2024
@atscott atscott changed the title feat(core): Add 'flush' parameter to fakeAsync to flush after the test feat(zone.js): Add 'flush' parameter to fakeAsync to flush after the test Aug 2, 2024
…er the test

From the internal issue on the matter:

> When using the standard Jasmine version of it promises returned by the body function are automatically awaited. The Catalyst version of it is fake-async, so awaiting the promise does not make sense; however it would be nice if Catalyst automatically flushed the promise to replicate the experience of using standard it. This would allow users to do the following:

```
it('should fail later', async () => {
  await new Promise(r => setTimeout(r));
  fail('failure');
});
```
> In Catalyst today the above test will pass. If this proposal to automatically flush the resulting promise were implemented it would fail.

Flushing after the tests complete has been the default behavior inside
Google since 2020. Very few tests remain that use the old behavior of
only flushing microtasks. The example above would actually fail with
`fakeAsync` due to the pending timer, but the argument still remains the
same. We might as well just flush if we're going to fail the test
anyways by throwing if there's no flush at the end.
@thePunderWoman
Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit 99d679d.

The changes were merged into the following branches: main

@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime detected: feature PR contains a feature commit merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants