Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test(core): ensure async tests are awaited properly #54801

Closed
wants to merge 4 commits into from
Closed

test(core): ensure async tests are awaited properly #54801

wants to merge 4 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Mar 10, 2024

@JoostK JoostK added target: rc This PR is targeted for the next release-candidate area: core Issues related to the framework runtime labels Mar 10, 2024
@ngbot ngbot bot added this to the Backlog milestone Mar 10, 2024
@JoostK
test(core): ensure async tests are awaited properly
20f85b9 
The assertion in `packages/core/test/acceptance/after_render_hook_spec.ts:165` was prone to flakes,
where Jasmine could frequently report an error:

```
Error: 'expect' was used when there was no current spec, this could be because an asynchronous test timed out
    at Env.expect (node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1945:15)
    at expect (node_modules/jasmine-core/lib/jasmine-core/jasmine.js:8267:18)
    at file:///packages/core/test/acceptance/after_render_hook_spec.ts:165:12
```

This happens because `wrapTestFn` checks for an exact type of `Promise`, which may have been patched by zone.js
such that the `instanceof` condition is dependent on whether zone.js has patched the `Promise` constructor.
@JoostK JoostK force-pushed the core/flaky-after-next-render branch from 0c427a4 to 505a633 Compare March 10, 2024 21:48
@JoostK JoostK marked this pull request as ready for review March 10, 2024 22:08
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 10, 2024
JoostK
JoostK commented Mar 10, 2024
View reviewed changes
packages/private/testing/src/utils.ts Outdated
return function (done: DoneFn) {
if (typeof blockFn === 'function') {
elementGetter().innerHTML = html;
const blockReturn = blockFn();
if (blockReturn instanceof Promise) {
if (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at it again, it may be possible to drop the done parameter altogether and just return the promise to Jasmine for it to take care of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Jasmine recognises returned promises (I assume it does) - than this makes sense to me

@JoostK JoostK mentioned this pull request Mar 11, 2024
Closed
devversion
devversion approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Yeah, I wonder if we can just return a promise instead, but can follow-up on that

@devversion
Copy link
Member

Caretaker note: Existing failures that got fixed via: #54791

@devversion devversion 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 Mar 11, 2024
pkozlowski-opensource
pkozlowski-opensource approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I agree that we could just return a promise but let's merge it as-is to unblock the CI / local tests

@pkozlowski-opensource pkozlowski-opensource removed the request for review from jessicajaniuk March 11, 2024 14:11
@pkozlowski-opensource pkozlowski-opensource added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 11, 2024
@JoostK
fixup! test(core): ensure async tests are awaited properly
68b337a
@JoostK
refactor(core): properly type withBody and withHead testing helpers
e9223ef 
This commit addresses a typing mismatch, where these functions were declared to return whichever
value their callback returned, but this was inaccurate: it's always a test callback function
with `done` argument.
@JoostK
refactor(core): avoid additional closure for queued microtask callback
6f7843c 
The `runCallbackOnce` closure is declared not to have any parameters itself, so it is
compatible as `queueMicrotask` callback without the extra closure. This reduces the call
stack by a frame and avoids the extra closure allocation.
@JoostK JoostK force-pushed the core/flaky-after-next-render branch from 505a633 to 6f7843c Compare March 11, 2024 15:45
@atscott
Copy link
Contributor

atscott commented Mar 11, 2024

This PR was merged into the repository by commit d269c88.

@atscott atscott closed this in 0bcaa0e Mar 11, 2024
atscott pushed a commit that referenced this pull request Mar 11, 2024
@JoostK @atscott
refactor(core): properly type withBody and withHead testing helpe…
fa315d4 
…rs (#54801)

This commit addresses a typing mismatch, where these functions were declared to return whichever
value their callback returned, but this was inaccurate: it's always a test callback function
with `done` argument.

PR Close #54801
atscott pushed a commit that referenced this pull request Mar 11, 2024
@JoostK @atscott
refactor(core): avoid additional closure for queued microtask callback (
d269c88 
#54801)

The `runCallbackOnce` closure is declared not to have any parameters itself, so it is
compatible as `queueMicrotask` callback without the extra closure. This reduces the call
stack by a frame and avoids the extra closure allocation.

PR Close #54801
atscott pushed a commit that referenced this pull request Mar 11, 2024
@JoostK @atscott
test(core): ensure async tests are awaited properly (#54801)
b6a7d50 
The assertion in `packages/core/test/acceptance/after_render_hook_spec.ts:165` was prone to flakes,
where Jasmine could frequently report an error:

```
Error: 'expect' was used when there was no current spec, this could be because an asynchronous test timed out
    at Env.expect (node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1945:15)
    at expect (node_modules/jasmine-core/lib/jasmine-core/jasmine.js:8267:18)
    at file:///packages/core/test/acceptance/after_render_hook_spec.ts:165:12
```

This happens because `wrapTestFn` checks for an exact type of `Promise`, which may have been patched by zone.js
such that the `instanceof` condition is dependent on whether zone.js has patched the `Promise` constructor.

PR Close #54801
atscott pushed a commit that referenced this pull request Mar 11, 2024
@JoostK @atscott
refactor(core): properly type withBody and withHead testing helpe…
e82173e 
…rs (#54801)

This commit addresses a typing mismatch, where these functions were declared to return whichever
value their callback returned, but this was inaccurate: it's always a test callback function
with `done` argument.

PR Close #54801
atscott pushed a commit that referenced this pull request Mar 11, 2024
@JoostK @atscott
refactor(core): avoid additional closure for queued microtask callback (
6ce8fc8 
#54801)

The `runCallbackOnce` closure is declared not to have any parameters itself, so it is
compatible as `queueMicrotask` callback without the extra closure. This reduces the call
stack by a frame and avoids the extra closure allocation.

PR Close #54801
@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 Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devversion devversion devversion approved these changes

@pkozlowski-opensource pkozlowski-opensource Awaiting requested review from pkozlowski-opensource

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: rc This PR is targeted for the next release-candidate
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JoostK @devversion @atscott @pkozlowski-opensource