Skip to content

Conversation

@atscott
Copy link
Contributor

@atscott atscott commented Aug 2, 2024

…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.

@pullapprove pullapprove bot requested a review from JiaLiPassion August 2, 2024 16:28
@atscott atscott added the target: minor This PR is targeted for the next minor release label Aug 2, 2024
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Aug 2, 2024
@atscott atscott added area: core Issues related to the framework runtime and removed detected: feature PR contains a feature commit labels Aug 2, 2024
@angular-robot angular-robot bot added the area: core Issues related to the framework runtime label Aug 2, 2024
@ngbot ngbot bot added this to the Backlog milestone Aug 2, 2024
…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.
@atscott atscott force-pushed the fakeasyncflushcore branch from 08469aa to 5619f03 Compare August 2, 2024 16:31
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Aug 2, 2024
@atscott atscott added the action: merge The PR is ready for merge by the caretaker label Aug 5, 2024
@thePunderWoman thePunderWoman added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 5, 2024
@thePunderWoman
Copy link
Contributor

caretaker note: this is safe to merge with current reviewers

@thePunderWoman
Copy link
Contributor

This PR was merged into the repository by commit f7918f5.

The changes were merged into the following branches: main

@angular-automatic-lock-bot
Copy link

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 5, 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.

2 participants