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.

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

ci: attempt to increase stability of saucelabs legacy job #44281

Closed

Conversation

devversion
Copy link
Member

@devversion devversion commented Nov 25, 2021

See individual commits

@google-cla google-cla bot added the cla: yes label Nov 25, 2021
@devversion devversion force-pushed the ci/increase-stability-saucelabs branch from 3df993d to 8042f4b Compare November 25, 2021 19:56
karma-js.conf.js Outdated Show resolved Hide resolved
@devversion devversion force-pushed the ci/increase-stability-saucelabs branch 3 times, most recently from d713046 to 85e5fe4 Compare November 26, 2021 18:35
App, {injector: defs.ngModule ? createInjector(defs.ngModule) : undefined});
expect(fixture.html).toEqual('<parent><view-child>view-child</view-child></parent>');

fixture.destroy();
Copy link
Member Author

Choose a reason for hiding this comment

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

This helper has just been moved from the spec file to a shared helper file. The only addition is this fixture.destroy call here; allowing us to also test that destroy hooks are invoked properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we can have an acceptance test instead (the one in the packages/core/test/acceptance folder) vs creating a new test that would use hand-written generated code (which we try to avoid in general)?

@devversion devversion force-pushed the ci/increase-stability-saucelabs branch 8 times, most recently from c869ba3 to 03207f9 Compare November 27, 2021 13:41
devversion and others added 6 commits November 27, 2021 15:05
This is an attempt to increase the stability of the Saucelabs legacy
job by using an emulator recommended by the Saucelabs platform
configurator, explicitly specifying the appium server version etc.
Bundle spec files similar to how it is done within the Angular
Components repo. This should simplify the setup and also speed
up the Saucelab job as only a single spec bundle would need to be
downloaded, compared to having to load hundreds of files through the
Saucelabs tunnel.

Also makes a couple of tests more robust with the emulators/and accounts
for ES2015 test runner changes. The tests should be less reluctant to
such build process changes.

Note for reviewers: Some imports have been simplified here. This work
came from Joey's original WIP for this. It's unclear to me whether this
is still needed, but it sounded like this was necessary for the ESBuild
bundling to work. I have robusted the module resolution plugin though,
so I doubt it's still needed. At the same time though: Not worth
reverting/trying as these changes are nice to have anyway!

Co-Authored-By: Joey Perrott <josephperrott@gmail.com>
Co-Authored-By: Paul Gschwendtner <paulgschwendtner@gmail.com>
…rwardRef`

Fixes an issue where destroy hooks are not set up for `useClass` providers
that rely on `forwardRef` for passing the actual class reference.

Currently the destroy hooks are not captured because forward refs are
not resolved for class providers. In ES2015+ this issue becomes even
more critical if arrow-functions are used for

e.g. `useClass: () => forwardRef(..)`.

This is because arrow functions do not have a prototype, unlike in ES5
where the function assigned to `useClass` would always have a prototype.
In ES2015+ a runtime exception as followed is returned (and doesn't give
any useful indication of the forwardRef issue):

```
TypeError: Cannot read properties of undefined (reading 'ngOnDestroy')
  at registerDestroyHooksIfSupported (dist/packages/core/src/render3/di_setup.ts:196:35 <- dist/legacy-test-bundle.spec.js:41508:37)
  at resolveProvider (dist/packages/core/src/render3/di_setup.ts:153:9 <- dist/legacy-test-bundle.spec.js:41485:11)
  at resolveProvider (dist/packages/core/src/render3/di_setup.ts:70:7 <- dist/legacy-test-bundle.spec.js:41445:9)
  at providersResolver (dist/packages/core/src/render3/di_setup.ts:54:5 <- dist/legacy-test-bundle.spec.js:41438:7)
  at Object.definition.providersResolver (dist/packages/core/src/render3/features/providers_feature.ts:48:18 <- dist/legacy-test-bundle.spec.js:41600:16)
  at resolveDirectives (dist/packages/core/src/render3/instructions/shared.ts:1179:40 <- dist/legacy-test-bundle.spec.js:35611:17)
  at elementStartFirstCreatePass (dist/packages/core/src/render3/instructions/element.ts:40:7 <- dist/legacy-test-bundle.spec.js:38432:27)
  at ɵɵelementStart (dist/packages/core/src/render3/instructions/element.ts:87:7 <- dist/legacy-test-bundle.spec.js:38453:43)
  at ɵɵelement (dist/packages/core/src/render3/instructions/element.ts:180:3 <- dist/legacy-test-bundle.spec.js:38510:5)
  at MultipleFormControls_Template (ng:///MultipleFormControls.js:9:9)
```
Re-adds the accidentally deleted downlevel decorator transform test.
…ctor parameters for nested classes

The downlevel decorator transform (commonly used in the CLI and other
tooling of the ecosystem for enabling JIT ES2015+), is currently
incorrectly dealing with nested classes.

The transform will accidentally visit nested classes (in a constructor)
multiple times and generate duplicated instances of the `ctorParameters`
fields. This does not sound like an issue at first, but the duplicated
`ctorParameters` fields will miss significant type information that has
been elided by the first visit, resulting in generated code like the
following:

```js
      let MyClass = /* @__PURE__ */ __name(class MyClass extends UpgradeNg1ComponentAdapter {
        constructor(scope, injector3, elementRef) {
        }
      }, "MyClass");
      MyClass.ctorParameters = () => [
        { type: void 0, decorators: [{ type: Inject, args: [$SCOPE] }] },
        { type: Injector },
        { type: ElementRef }
      ];
      MyClass.ctorParameters = () => [
        { type: void 0 }, // <---- NOTE!
        { type: Injector },
        { type: ElementRef }
      ];
```
Removes unused router karma legacy test setup that
just caused unnecessary confusion.
@devversion devversion force-pushed the ci/increase-stability-saucelabs branch from f5b4d32 to bf60028 Compare November 27, 2021 14:07
Previously, when we did not bundle tests, we also had the jasmine
default timeout increased to 15000ms. This commit re-introduces this
increased timeout by also maintaining a custom test init file for the
legacy saucelab jobs. This is desirable as all of the Saucelabs test
code is local in the `tools/legacy-saucelabs` folder.
@devversion devversion force-pushed the ci/increase-stability-saucelabs branch from bf60028 to 604de32 Compare November 27, 2021 14:11
@devversion devversion marked this pull request as ready for review November 27, 2021 14:38
@AndrewKushnir AndrewKushnir 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 Nov 30, 2021
@AndrewKushnir
Copy link
Contributor

Merge-assistance: this PR is ready for merge. Alex's approval covers the remaining fw-benchmarks group as well (Alex will be a part of that group once PR #44304 is merged).

@dylhunn
Copy link
Contributor

dylhunn commented Nov 30, 2021

This PR was merged into the repository by commit 0fcf18d.

dylhunn pushed a commit that referenced this pull request Nov 30, 2021
This is an attempt to increase the stability of the Saucelabs legacy
job by using an emulator recommended by the Saucelabs platform
configurator, explicitly specifying the appium server version etc.

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
Bundle spec files similar to how it is done within the Angular
Components repo. This should simplify the setup and also speed
up the Saucelab job as only a single spec bundle would need to be
downloaded, compared to having to load hundreds of files through the
Saucelabs tunnel.

Also makes a couple of tests more robust with the emulators/and accounts
for ES2015 test runner changes. The tests should be less reluctant to
such build process changes.

Note for reviewers: Some imports have been simplified here. This work
came from Joey's original WIP for this. It's unclear to me whether this
is still needed, but it sounded like this was necessary for the ESBuild
bundling to work. I have robusted the module resolution plugin though,
so I doubt it's still needed. At the same time though: Not worth
reverting/trying as these changes are nice to have anyway!

Co-Authored-By: Joey Perrott <josephperrott@gmail.com>
Co-Authored-By: Paul Gschwendtner <paulgschwendtner@gmail.com>

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
…rwardRef` (#44281)

Fixes an issue where destroy hooks are not set up for `useClass` providers
that rely on `forwardRef` for passing the actual class reference.

Currently the destroy hooks are not captured because forward refs are
not resolved for class providers. In ES2015+ this issue becomes even
more critical if arrow-functions are used for

e.g. `useClass: () => forwardRef(..)`.

This is because arrow functions do not have a prototype, unlike in ES5
where the function assigned to `useClass` would always have a prototype.
In ES2015+ a runtime exception as followed is returned (and doesn't give
any useful indication of the forwardRef issue):

```
TypeError: Cannot read properties of undefined (reading 'ngOnDestroy')
  at registerDestroyHooksIfSupported (dist/packages/core/src/render3/di_setup.ts:196:35 <- dist/legacy-test-bundle.spec.js:41508:37)
  at resolveProvider (dist/packages/core/src/render3/di_setup.ts:153:9 <- dist/legacy-test-bundle.spec.js:41485:11)
  at resolveProvider (dist/packages/core/src/render3/di_setup.ts:70:7 <- dist/legacy-test-bundle.spec.js:41445:9)
  at providersResolver (dist/packages/core/src/render3/di_setup.ts:54:5 <- dist/legacy-test-bundle.spec.js:41438:7)
  at Object.definition.providersResolver (dist/packages/core/src/render3/features/providers_feature.ts:48:18 <- dist/legacy-test-bundle.spec.js:41600:16)
  at resolveDirectives (dist/packages/core/src/render3/instructions/shared.ts:1179:40 <- dist/legacy-test-bundle.spec.js:35611:17)
  at elementStartFirstCreatePass (dist/packages/core/src/render3/instructions/element.ts:40:7 <- dist/legacy-test-bundle.spec.js:38432:27)
  at ɵɵelementStart (dist/packages/core/src/render3/instructions/element.ts:87:7 <- dist/legacy-test-bundle.spec.js:38453:43)
  at ɵɵelement (dist/packages/core/src/render3/instructions/element.ts:180:3 <- dist/legacy-test-bundle.spec.js:38510:5)
  at MultipleFormControls_Template (ng:///MultipleFormControls.js:9:9)
```

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
Re-adds the accidentally deleted downlevel decorator transform test.

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
…ctor parameters for nested classes (#44281)

The downlevel decorator transform (commonly used in the CLI and other
tooling of the ecosystem for enabling JIT ES2015+), is currently
incorrectly dealing with nested classes.

The transform will accidentally visit nested classes (in a constructor)
multiple times and generate duplicated instances of the `ctorParameters`
fields. This does not sound like an issue at first, but the duplicated
`ctorParameters` fields will miss significant type information that has
been elided by the first visit, resulting in generated code like the
following:

```js
      let MyClass = /* @__PURE__ */ __name(class MyClass extends UpgradeNg1ComponentAdapter {
        constructor(scope, injector3, elementRef) {
        }
      }, "MyClass");
      MyClass.ctorParameters = () => [
        { type: void 0, decorators: [{ type: Inject, args: [$SCOPE] }] },
        { type: Injector },
        { type: ElementRef }
      ];
      MyClass.ctorParameters = () => [
        { type: void 0 }, // <---- NOTE!
        { type: Injector },
        { type: ElementRef }
      ];
```

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
Removes unused router karma legacy test setup that
just caused unnecessary confusion.

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
Previously, when we did not bundle tests, we also had the jasmine
default timeout increased to 15000ms. This commit re-introduces this
increased timeout by also maintaining a custom test init file for the
legacy saucelab jobs. This is desirable as all of the Saucelabs test
code is local in the `tools/legacy-saucelabs` folder.

PR Close #44281
@dylhunn dylhunn closed this in 61e0234 Nov 30, 2021
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
Bundle spec files similar to how it is done within the Angular
Components repo. This should simplify the setup and also speed
up the Saucelab job as only a single spec bundle would need to be
downloaded, compared to having to load hundreds of files through the
Saucelabs tunnel.

Also makes a couple of tests more robust with the emulators/and accounts
for ES2015 test runner changes. The tests should be less reluctant to
such build process changes.

Note for reviewers: Some imports have been simplified here. This work
came from Joey's original WIP for this. It's unclear to me whether this
is still needed, but it sounded like this was necessary for the ESBuild
bundling to work. I have robusted the module resolution plugin though,
so I doubt it's still needed. At the same time though: Not worth
reverting/trying as these changes are nice to have anyway!

Co-Authored-By: Joey Perrott <josephperrott@gmail.com>
Co-Authored-By: Paul Gschwendtner <paulgschwendtner@gmail.com>

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
…rwardRef` (#44281)

Fixes an issue where destroy hooks are not set up for `useClass` providers
that rely on `forwardRef` for passing the actual class reference.

Currently the destroy hooks are not captured because forward refs are
not resolved for class providers. In ES2015+ this issue becomes even
more critical if arrow-functions are used for

e.g. `useClass: () => forwardRef(..)`.

This is because arrow functions do not have a prototype, unlike in ES5
where the function assigned to `useClass` would always have a prototype.
In ES2015+ a runtime exception as followed is returned (and doesn't give
any useful indication of the forwardRef issue):

```
TypeError: Cannot read properties of undefined (reading 'ngOnDestroy')
  at registerDestroyHooksIfSupported (dist/packages/core/src/render3/di_setup.ts:196:35 <- dist/legacy-test-bundle.spec.js:41508:37)
  at resolveProvider (dist/packages/core/src/render3/di_setup.ts:153:9 <- dist/legacy-test-bundle.spec.js:41485:11)
  at resolveProvider (dist/packages/core/src/render3/di_setup.ts:70:7 <- dist/legacy-test-bundle.spec.js:41445:9)
  at providersResolver (dist/packages/core/src/render3/di_setup.ts:54:5 <- dist/legacy-test-bundle.spec.js:41438:7)
  at Object.definition.providersResolver (dist/packages/core/src/render3/features/providers_feature.ts:48:18 <- dist/legacy-test-bundle.spec.js:41600:16)
  at resolveDirectives (dist/packages/core/src/render3/instructions/shared.ts:1179:40 <- dist/legacy-test-bundle.spec.js:35611:17)
  at elementStartFirstCreatePass (dist/packages/core/src/render3/instructions/element.ts:40:7 <- dist/legacy-test-bundle.spec.js:38432:27)
  at ɵɵelementStart (dist/packages/core/src/render3/instructions/element.ts:87:7 <- dist/legacy-test-bundle.spec.js:38453:43)
  at ɵɵelement (dist/packages/core/src/render3/instructions/element.ts:180:3 <- dist/legacy-test-bundle.spec.js:38510:5)
  at MultipleFormControls_Template (ng:///MultipleFormControls.js:9:9)
```

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
Re-adds the accidentally deleted downlevel decorator transform test.

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
…ctor parameters for nested classes (#44281)

The downlevel decorator transform (commonly used in the CLI and other
tooling of the ecosystem for enabling JIT ES2015+), is currently
incorrectly dealing with nested classes.

The transform will accidentally visit nested classes (in a constructor)
multiple times and generate duplicated instances of the `ctorParameters`
fields. This does not sound like an issue at first, but the duplicated
`ctorParameters` fields will miss significant type information that has
been elided by the first visit, resulting in generated code like the
following:

```js
      let MyClass = /* @__PURE__ */ __name(class MyClass extends UpgradeNg1ComponentAdapter {
        constructor(scope, injector3, elementRef) {
        }
      }, "MyClass");
      MyClass.ctorParameters = () => [
        { type: void 0, decorators: [{ type: Inject, args: [$SCOPE] }] },
        { type: Injector },
        { type: ElementRef }
      ];
      MyClass.ctorParameters = () => [
        { type: void 0 }, // <---- NOTE!
        { type: Injector },
        { type: ElementRef }
      ];
```

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
Removes unused router karma legacy test setup that
just caused unnecessary confusion.

PR Close #44281
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
Previously, when we did not bundle tests, we also had the jasmine
default timeout increased to 15000ms. This commit re-introduces this
increased timeout by also maintaining a custom test init file for the
legacy saucelab jobs. This is desirable as all of the Saucelabs test
code is local in the `tools/legacy-saucelabs` folder.

PR Close #44281
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
)

This is an attempt to increase the stability of the Saucelabs legacy
job by using an emulator recommended by the Saucelabs platform
configurator, explicitly specifying the appium server version etc.

PR Close angular#44281
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
Bundle spec files similar to how it is done within the Angular
Components repo. This should simplify the setup and also speed
up the Saucelab job as only a single spec bundle would need to be
downloaded, compared to having to load hundreds of files through the
Saucelabs tunnel.

Also makes a couple of tests more robust with the emulators/and accounts
for ES2015 test runner changes. The tests should be less reluctant to
such build process changes.

Note for reviewers: Some imports have been simplified here. This work
came from Joey's original WIP for this. It's unclear to me whether this
is still needed, but it sounded like this was necessary for the ESBuild
bundling to work. I have robusted the module resolution plugin though,
so I doubt it's still needed. At the same time though: Not worth
reverting/trying as these changes are nice to have anyway!

Co-Authored-By: Joey Perrott <josephperrott@gmail.com>
Co-Authored-By: Paul Gschwendtner <paulgschwendtner@gmail.com>

PR Close angular#44281
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
…rwardRef` (angular#44281)

Fixes an issue where destroy hooks are not set up for `useClass` providers
that rely on `forwardRef` for passing the actual class reference.

Currently the destroy hooks are not captured because forward refs are
not resolved for class providers. In ES2015+ this issue becomes even
more critical if arrow-functions are used for

e.g. `useClass: () => forwardRef(..)`.

This is because arrow functions do not have a prototype, unlike in ES5
where the function assigned to `useClass` would always have a prototype.
In ES2015+ a runtime exception as followed is returned (and doesn't give
any useful indication of the forwardRef issue):

```
TypeError: Cannot read properties of undefined (reading 'ngOnDestroy')
  at registerDestroyHooksIfSupported (dist/packages/core/src/render3/di_setup.ts:196:35 <- dist/legacy-test-bundle.spec.js:41508:37)
  at resolveProvider (dist/packages/core/src/render3/di_setup.ts:153:9 <- dist/legacy-test-bundle.spec.js:41485:11)
  at resolveProvider (dist/packages/core/src/render3/di_setup.ts:70:7 <- dist/legacy-test-bundle.spec.js:41445:9)
  at providersResolver (dist/packages/core/src/render3/di_setup.ts:54:5 <- dist/legacy-test-bundle.spec.js:41438:7)
  at Object.definition.providersResolver (dist/packages/core/src/render3/features/providers_feature.ts:48:18 <- dist/legacy-test-bundle.spec.js:41600:16)
  at resolveDirectives (dist/packages/core/src/render3/instructions/shared.ts:1179:40 <- dist/legacy-test-bundle.spec.js:35611:17)
  at elementStartFirstCreatePass (dist/packages/core/src/render3/instructions/element.ts:40:7 <- dist/legacy-test-bundle.spec.js:38432:27)
  at ɵɵelementStart (dist/packages/core/src/render3/instructions/element.ts:87:7 <- dist/legacy-test-bundle.spec.js:38453:43)
  at ɵɵelement (dist/packages/core/src/render3/instructions/element.ts:180:3 <- dist/legacy-test-bundle.spec.js:38510:5)
  at MultipleFormControls_Template (ng:///MultipleFormControls.js:9:9)
```

PR Close angular#44281
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
Re-adds the accidentally deleted downlevel decorator transform test.

PR Close angular#44281
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
…ctor parameters for nested classes (angular#44281)

The downlevel decorator transform (commonly used in the CLI and other
tooling of the ecosystem for enabling JIT ES2015+), is currently
incorrectly dealing with nested classes.

The transform will accidentally visit nested classes (in a constructor)
multiple times and generate duplicated instances of the `ctorParameters`
fields. This does not sound like an issue at first, but the duplicated
`ctorParameters` fields will miss significant type information that has
been elided by the first visit, resulting in generated code like the
following:

```js
      let MyClass = /* @__PURE__ */ __name(class MyClass extends UpgradeNg1ComponentAdapter {
        constructor(scope, injector3, elementRef) {
        }
      }, "MyClass");
      MyClass.ctorParameters = () => [
        { type: void 0, decorators: [{ type: Inject, args: [$SCOPE] }] },
        { type: Injector },
        { type: ElementRef }
      ];
      MyClass.ctorParameters = () => [
        { type: void 0 }, // <---- NOTE!
        { type: Injector },
        { type: ElementRef }
      ];
```

PR Close angular#44281
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
Removes unused router karma legacy test setup that
just caused unnecessary confusion.

PR Close angular#44281
dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
…lar#44281)

Previously, when we did not bundle tests, we also had the jasmine
default timeout increased to 15000ms. This commit re-introduces this
increased timeout by also maintaining a custom test init file for the
legacy saucelab jobs. This is desirable as all of the Saucelabs test
code is local in the `tools/legacy-saucelabs` folder.

PR Close angular#44281
@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 Dec 31, 2021
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: build & ci Related the build and CI infrastructure of the project cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note PullApprove: disable target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants