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

fix(zone.js): patch global instead of Mocha object #45047

Closed
wants to merge 2 commits into from
Closed

fix(zone.js): patch global instead of Mocha object #45047

wants to merge 2 commits into from
+9,827 −16

Conversation

JiaLiPassion
Copy link
Contributor

@JiaLiPassion JiaLiPassion commented Feb 11, 2022 •
edited
Loading

Close #42834

In the new version fo Mocha, all global test functions are from global
object instead of Mocha object. Adn the current zone.js Mocha
patch's logic looks like this.

global.describe = Mocha.describe = function() {
  return originalMochaDescribe.apply(this, arguments);
}

and originalMochaDescribe is the unpathced Mocha implementation
looks like this

function describe() {
  return context.describe(...);
}

And the context will finally delegate to global.describe(),
so the current zone.js patch causes infinite loop.

This commit will not patch function of Mocha object any longer.

@pullapprove pullapprove bot requested review from atscott and gkalpak February 11, 2022 04:44
@JiaLiPassion
fix(zone.js): patch global instead of Mocha object
09103a9 
Close angular#42834

In the new version fo Mocha, all global test functions are from `global`
object instead of `Mocha` object. Adn the current `zone.js` Mocha
patch's logic looks like this.

```
global.describe = Mocha.describe = function() {
  return originalMochaDescribe.apply(this, arguments);
}
```

and `originalMochaDescribe` is the unpathced Mocha implementation
looks like this

```
function describe() {
  return context.describe(...);
}
```

And the `context` will finally delegate to `global.describe()`,
so the current `zone.js` patch causes infinite loop.

This commit will not patch function of `Mocha` object any longer.
@JiaLiPassion JiaLiPassion added the area: zones Issues related to zone.js label Feb 11, 2022
@ngbot ngbot bot added this to the Backlog milestone Feb 11, 2022
@JiaLiPassion JiaLiPassion added target: minor This PR is targeted for the next minor release risk: low labels Feb 11, 2022
@JiaLiPassion
test(zone.js): add integration test for zone.js Mocha patch
9069560 
Add integration test for `Mocha` patch from `zone.js` to verify the
issue angular#42384 is fixed
atscott
atscott approved these changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

reviewed-for: integration-tests

Verified that tests ran and passed

//integration/cli-hello-world-mocha:test                                 PASSED in 79.9s

https://app.circleci.com/pipelines/github/angular/angular/42590/workflows/ba543a9b-bf28-4af5-8465-1a46c3242ca2/jobs/1120899

@atscott
Copy link
Contributor

atscott commented Feb 14, 2022

@JiaLiPassion Is there a reason this is marked as minor and not patch? Since it's a fix, I would have thought we would want to merge this to patch as well.

@JiaLiPassion JiaLiPassion added target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Feb 14, 2022
@JiaLiPassion
Copy link
Contributor Author

@atscott , thank you for the review, I have updated the target to patch.

@jessicajaniuk jessicajaniuk removed the request for review from gkalpak February 16, 2022 19:09
@jessicajaniuk jessicajaniuk added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Feb 16, 2022
@alxhub
Copy link
Member

alxhub commented Feb 16, 2022

This PR was merged into the repository by commit ee381da.

@alxhub alxhub closed this in 8efbdb5 Feb 16, 2022
alxhub pushed a commit that referenced this pull request Feb 16, 2022
@JiaLiPassion @alxhub
test(zone.js): add integration test for zone.js Mocha patch (#45047)
ee381da 
Add integration test for `Mocha` patch from `zone.js` to verify the
issue #42384 is fixed

PR Close #45047
alxhub pushed a commit that referenced this pull request Feb 16, 2022
@JiaLiPassion @alxhub
fix(zone.js): patch global instead of Mocha object (#45047)
2ec696a 
Close #42834

In the new version fo Mocha, all global test functions are from `global`
object instead of `Mocha` object. Adn the current `zone.js` Mocha
patch's logic looks like this.

```
global.describe = Mocha.describe = function() {
  return originalMochaDescribe.apply(this, arguments);
}
```

and `originalMochaDescribe` is the unpathced Mocha implementation
looks like this

```
function describe() {
  return context.describe(...);
}
```

And the `context` will finally delegate to `global.describe()`,
so the current `zone.js` patch causes infinite loop.

This commit will not patch function of `Mocha` object any longer.

PR Close #45047
alxhub pushed a commit that referenced this pull request Feb 16, 2022
@JiaLiPassion @alxhub
test(zone.js): add integration test for zone.js Mocha patch (#45047)
62cec95 
Add integration test for `Mocha` patch from `zone.js` to verify the
issue #42384 is fixed

PR Close #45047
@dpraul
Copy link

dpraul commented Mar 2, 2022 •
edited
Loading

Is there a new release scheduled for zone.js so that we can utilize this patch?

Just saw that this question is answered in the linked issue - sorry for the noise.

crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 8, 2022
Update angular monorepo (#1197)
745ec5d 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fanimations/13.2.4/13.2.5) |
| [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fcommon/13.2.4/13.2.5) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/13.2.4/13.2.5) |
| [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/13.2.4/13.2.5) |
| [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fcore/13.2.4/13.2.5) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fforms/13.2.4/13.2.5) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/13.2.4/13.2.5) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/13.2.4/13.2.5) |
| [@angular/router](https://github.com/angular/angular) | dependencies | patch | [`13.2.4` -> `13.2.5`](https://renovatebot.com/diffs/npm/@angular%2frouter/13.2.4/13.2.5) |
| [zone.js](https://github.com/angular/angular) ([changelog](https://github.com/angular/angular/blob/master/packages/zone.js/CHANGELOG.md)) | dependencies | patch | [`0.11.4` -> `0.11.5`](https://renovatebot.com/diffs/npm/zone.js/0.11.4/0.11.5) |

---

### Release Notes

<details>
<summary>angular/angular (@&#8203;angular/animations)</summary>

### [`v13.2.5`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1325-2022-03-02)

[Compare Source](angular/angular@13.2.4...13.2.5)

##### animations

| Commit | Type | Description |
| -- | -- | -- |
| [6c61d20476](angular/angular@6c61d20) | fix | allow animations with unsupported CSS properties ([#&#8203;45185](angular/angular#45185)) |

##### common

| Commit | Type | Description |
| -- | -- | -- |
| [64da1daa78](angular/angular@64da1da) | fix | canceled JSONP requests won't throw console error with missing callback function ([#&#8203;36807](angular/angular#36807)) |
| [56ca7d385b](angular/angular@56ca7d3) | perf | make `NgLocalization` token tree-shakable ([#&#8203;45118](angular/angular#45118)) ([#&#8203;45226](angular/angular#45226)) |

##### compiler-cli

| Commit | Type | Description |
| -- | -- | -- |
| [6c906a5bb9](angular/angular@6c906a5) | fix | Support resolve animation name from the DTS ([#&#8203;45169](angular/angular#45169)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [e8fd452bd2](angular/angular@e8fd452) | fix | remove individual commands for updating gold files ([#&#8203;45198](angular/angular#45198)) |
| [82d772857c](angular/angular@82d7728) | perf | make `Compiler`, `ApplicationRef` and `ApplicationInitStatus` tree-shakable ([#&#8203;45102](angular/angular#45102)) ([#&#8203;45222](angular/angular#45222)) |
| [71ff12c1cc](angular/angular@71ff12c) | perf | make `LOCALE_ID` and other tokens from `ApplicationModule` tree-shakable ([#&#8203;45102](angular/angular#45102)) ([#&#8203;45222](angular/angular#45222)) |

##### localize

| Commit | Type | Description |
| -- | -- | -- |
| [d388522745](angular/angular@d388522) | fix | avoid imports into `compiler-cli` package ([#&#8203;45180](angular/angular#45180)) |

#### Special Thanks

Andrew Kushnir, Andrew Scott, Charles Lyding, Guillaume Bonnet, Jessica Janiuk, JoostK, Martin Sikora, Paul Gschwendtner, Theodore Brown, dario-piotrowicz and ivanwonder

<!-- CHANGELOG SPLIT MARKER -->

</details>

<details>
<summary>angular/angular (zone.js)</summary>

### [`v0.11.5`](https://github.com/angular/angular/blob/HEAD/packages/zone.js/CHANGELOG.md#&#8203;0115-httpsgithubcomangularangularcomparezonejs-0114zonejs-0115-2022-03-03)

[Compare Source](angular/angular@zone.js-0.11.4...zone.js-0.11.5)

##### Bug Fixes

-   **zone.js:** async-test should only call done once ([#&#8203;45025](angular/angular#45025)) ([dea7234](angular/angular@dea7234))
-   **zone.js:** defineProperties should also set symbol props ([#&#8203;45098](angular/angular#45098)) ([b437d12](angular/angular@b437d12)), closes [#&#8203;44095](angular/angular#44095)
-   **zone.js:** fix several test cases which trigger `done()` multiple times ([#&#8203;45025](angular/angular#45025)) ([d5565cc](angular/angular@d5565cc))
-   **zone.js:** only one listener should also re-throw an error correctly ([#&#8203;41868](angular/angular#41868)) ([299f92c](angular/angular@299f92c)), closes [#&#8203;41867](angular/angular#41867) [/github.com/angular/angular/pull/41562#issuecomment-822696973](https://github.com//github.com/angular/angular/pull/41562/issues/issuecomment-822696973)
-   **zone.js:** patch global instead of Mocha object ([#&#8203;45047](angular/angular#45047)) ([8efbdb5](angular/angular@8efbdb5)), closes [#&#8203;42834](angular/angular#42834)
-   **zone.js:** should continue to executue listeners when throw error ([#&#8203;41562](angular/angular#41562)) ([008eaf3](angular/angular@008eaf3)), closes [#&#8203;41522](angular/angular#41522)
-   **zone.js:** update several flaky cases ([#&#8203;41526](angular/angular#41526)) ([25a83eb](angular/angular@25a83eb)), closes [#&#8203;41434](angular/angular#41434)

##### Features

-   **zone.js:** add Promise.any() implementation ([#&#8203;45064](angular/angular#45064)) ([4d494d2](angular/angular@4d494d2)), closes [#&#8203;44393](angular/angular#44393)
-   **zone.js:** update electron patch to support electron/remote 14 ([#&#8203;45073](angular/angular#45073)) ([d65706a](angular/angular@d65706a)), closes [#&#8203;43346](angular/angular#43346)

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1197
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@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 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: zones Issues related to zone.js merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: low target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

zone.js fails to patch mocha v8.3.1 and up
5 participants
@JiaLiPassion @atscott @alxhub @dpraul @jessicajaniuk