Skip to content

Conversation

@crisbeto
Copy link
Member

@crisbeto crisbeto commented Feb 1, 2022

Currently zone.js may end up attempting to call the done function of async tests more than once. This used to be a noop, but Jasmine now logs the following warning:

DEPRECATION: An asynchronous function called its 'done' callback more than once. This is a bug in the spec, beforeAll, beforeEach, afterAll, or afterEach function in question. This will be treated as an error in a future version. See<https://jasmine.github.io/tutorials/upgrading_to_Jasmine_4.0#deprecations-due-to-calling-done-multiple-times> for more information

These changes add a wrapper around the done function so it only gets called once, otherwise we risk triggering errors in future versions of Jasmine.

Note: I couldn't figure out a good way to add a test for this since it would be checking something that's part of the testing infrastructure.

@crisbeto crisbeto marked this pull request as ready for review February 1, 2022 11:22
@pullapprove pullapprove bot requested a review from JiaLiPassion February 1, 2022 11:22
@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: zones Issues related to zone.js target: patch This PR is targeted for the next patch release labels Feb 1, 2022
@ngbot ngbot bot modified the milestone: Backlog Feb 1, 2022
Currently zone.js may end up attempting to call the `done` function of async tests more than once. This used to be a noop, but Jasmine now logs the following warning:

```
An asynchronous function called its 'done' callback more than once. This is a bug in the spec, beforeAll, beforeEach, afterAll, or afterEach function in question. This will be treated as an error in a future version. See<https://jasmine.github.io/tutorials/upgrading_to_Jasmine_4.0#deprecations-due-to-calling-done-multiple-times> for more information
```

These changes add a wrapper around the `done` function so it only gets called once, otherwise we risk triggering errors in future versions of Jasmine.
@JiaLiPassion
Copy link
Contributor

@crisbeto , the changes LGTM, I just wonder is there a reproduce scenario or repo to describe the error case?

@crisbeto
Copy link
Member Author

crisbeto commented Feb 7, 2022

I noticed it both in the tests of this repo and the ones in angular/components. Usually you won't see it, because Bazel doesn't log anything if the test run passes. You can get it to show up by adding a test that always fails. I was testing like this:

  1. Add a failing test to packages/core/test/util_spec.ts. Something like it('should fail', () => expect(1).toBe(2));. The file doesn't really matter, as long as it's in the test target mentioned below.
  2. Run yarn bazel test packages/core/test:test.

@JiaLiPassion
Copy link
Contributor

@crisbeto , thank you for the reproduce, I did some debug and find out this maybe a bug of asyncTest in zone.js, I will try to fix it in another PR, I will let you know when it is ready for review.

@crisbeto
Copy link
Member Author

crisbeto commented Feb 9, 2022

So I had this fixed in aysnc-test.ts as well by only calling the finishCallback once here https://github.com/angular/angular/blob/master/packages/zone.js/lib/zone-spec/async-test.ts#L226. I ended up doing it in jasmine.ts instead, because it seemed like a more appropriate place for a fix that is specific to a warning generated by Jasmine.

@JiaLiPassion
Copy link
Contributor

@crisbeto , I just figured it out that the issue is a bug of AsyncTestZone, and I created a PR to fix it, #45025, I believe this fix will resolve the jasmine warning about the multiple done issue, and also resolve a lot of other flaky test issues . Please review. Thank you

@crisbeto
Copy link
Member Author

crisbeto commented Feb 9, 2022

Closing in favor of #45025.

@crisbeto crisbeto closed this Feb 9, 2022
@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 Mar 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: review The PR is still awaiting reviews from at least one requested reviewer area: zones Issues related to zone.js target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants