test: add failing test cases with single file transpilation for JIT transformers #2615

ahnpnl · 2024-07-22T21:47:02Z

Test plan

Green CI

Does this PR introduce a breaking change?

Yes
No

Other information

N.A.

ahnpnl · 2024-07-22T21:55:53Z

Hi @dgp1130, as discussed via DM before, I have copied most of tests from https://github.com/angular/angular/blob/main/packages/compiler-cli/src/ngtsc/transform/jit/test/downlevel_decorators_transform_spec.ts Some tests I found not needed for Jest so I didn't include them here. The amount of copied tests in this PR I think should be enough to cover all cases.

There are indeed a few cases where transpileModule doesn't work, which I mark them with it.failing. Would you please help me to take a look whenever there is time? Thank you!

dgp1130

Left some general thoughts on the failing tests. I don't know that I have great answers for why they're failing, but hopefully the extra context is helpful.

I do wonder how much of this is downlevel_decorators_transform being incompatible with isolatedModules and how much is just not taking the effort to support it. Would some of these failures be fixable upstream or at least gracefully degrade and/or give a better error message about what works?

dgp1130 · 2024-07-23T18:27:56Z

src/compiler/ng-jest-compiler.spec.ts

+                ];
+                exports.MyDir = MyDir = tslib_1.__decorate([
+                    (0, core_1.Directive)(),
+                    tslib_1.__metadata("design:paramtypes", Object])


My understanding of emitDecoratorMetadata is that it changes the output slightly based on whether or not the constructor parameter type is local or imported. It wants to emit tslib_1.__metadata("design:paramtypes", MyOtherClass), but TS can't introspect the type of an imported symbol with isolatedModules and doesn't know MyOtherClass is a class. Instead, TS needs to generate a small JS snippet to determine this at runtime. See the output from this example. Is that the output you get which causes the test failure?

Interestingly, tsc seems to add this snippet whether or not isolatedModules is enabled. In theory I can see that TS wouldn't know MyOtherClass is a class as opposed to an interface and maybe needs to pick Object because it can't be certain MyOtherClass is valid in a value reference position.

Yes the output from TS playground was exactly the output which caused the test failture. If we leave the output to be similar like TS playground, will that cause any issues when testing real application codes?

Is emitDecoratorMetadata: true still important to Angular apps these days? I remember somewhere I saw a commit from Angular which allows to disable emitDecoratorMetadata and application still works.

I think that emitted JS is probably fine? Hard to be 100% sure unfortunately.

ng new does not enable emitDecoratorMetadata in the scaffolded tsconfig.json files, so I suspect it's not required, but I wouldn't be surprised if there are other effects from enabling it. I can ask around and see if anyone has more context.

dgp1130 · 2024-07-23T18:43:31Z

src/compiler/ng-jest-compiler.spec.ts

+        it.failing(
+            'should allow for type-only references to be removed with `emitDecoratorMetadata` from custom decorators',
+            () => {
+                // This test only passes when using `import type { ExternalInterface } from './external-interface';`


This one makes sense to me because with isolatedModules TS shouldn't know that it's an interface and not eligible to be moved into a value reference position.

The output of this case is

exports.Foo = Foo; tslib_1.__decorate([ CustomDecorator(), tslib_1.__metadata(\"design:type\", Function), tslib_1.__metadata(\"design:paramtypes\", []), tslib_1.__metadata(\"design:returntype\", typeof (_a = typeof external_interface_1.ExternalInterface !== \"undefined\" && external_interface_1.ExternalInterface) === \"function\" ? _a : Object) ], Foo, \"test\", null);

which made the assertion expect(code).not.toContain('ExternalInterface'); fail.

Will the output like that cause any issues when testing real application codes?

typeof external_interface_1.ExternalInterface sounds wrong to me. Interfaces are erased by TypeScript, so this expression can't work at runtime. I would expect this to be something like Object in this scenario since it can't be any more specific about the type.

I don't think Angular uses design:returntype anywhere. I think we only care about constructor parameters. So it probably doesn't matter what's actually in the __metadata call, as long as it doesn't throw.

dgp1130 · 2024-07-23T18:46:43Z

src/compiler/ng-jest-compiler.spec.ts

+            // This test doesn't pass because `transpileModule` can't resolve `import *`
+            const { code, diagnostics } = transformCjs(`
+                import {Directive} from '@angular/core';
+                import * as externalFile from './other-file';


I'm surprised this would be different than import { MyOtherClass } from './other-file'; and makes me wonder if there's a bug in entityNameToExpression somewhere? We have special handling of aliased imports, I wonder if we also need special handling for namespaced imports?

I can confirm name import and default import work, only namespace import doesn't

src/compiler/ng-jest-compiler.spec.ts

dgp1130 · 2024-07-23T18:51:39Z

src/compiler/ng-jest-compiler.spec.ts

+                    { type: undefined, decorators: [{ type: core_1.Inject, args: ['$state',] }] },
+                    { type: undefined, decorators: [{ type: core_1.Inject, args: ['$overlay',] }] },
+                    { type: undefined, decorators: [{ type: core_1.Inject, args: ['$default',] }] },
+                    { type: undefined, decorators: [{ type: core_1.Inject, args: ['$keyCodes',] }] }


If type is expected to be undefined, then why are we trying to resolve the import *? It doesn't seem like we shouldn't need that type? I wonder if the correct behavior on resolution error would just be to fall back to any as the type (and emit undefined)?

This case produces very interesting output

Object.defineProperty(exports, \"__esModule\", { value: true }); exports.MyDir = void 0; const tslib_1 = require(\"tslib\"); const core_1 = require(\"@angular/core\"); const external_1 = require(\"./external\"); const external_2 = tslib_1.__importDefault(require(\"./external\")); let MyDir = class MyDir { constructor(param, other, fromDefaultImport, keyCodes) { } }; exports.MyDir = MyDir; MyDir.ctorParameters = () => [ { type: undefined, decorators: [{ type: core_1.Inject, args: ['$state',] }] }, { type: external_1.IOverlay, decorators: [{ type: core_1.Inject, args: ['$overlay',] }] }, { type: external_2.default, decorators: [{ type: core_1.Inject, args: ['$default',] }] }, { type: external_1.KeyCodes, decorators: [{ type: core_1.Inject, args: ['$keyCodes',] }] } ]; exports.MyDir = MyDir = tslib_1.__decorate([ (0, core_1.Directive)() ], MyDir);

I saw that the type was resolved correctly when using import name or default import but fail with import namespace.

Hmm, any of these could be an interface which wouldn't be valid in type though. I wonder if the type system is falling back to any because it can't be resolved and then we're assuming the import is a valid value reference. I wonder if the safer assumption would be to fall back to type: undefined in that scenario?

You were right. When I do typeChecker.getTypeOfSymbol(symbol), it gave me any.

In retrospect, any might not necessarily be a problem. Both type references and value references can be typed any, so that might be fine. I wonder if it's possible to ask TS whether or not each import is a value reference or a type reference. I'm guessing it's assuming imported symbols are a value reference when it can't resolve the import and we'd probably want to reduce that assumption to a type reference in this scenario.

Unfortunately I'm not very familiar with the TS API and docs are pretty lacking, so I'm not sure about the best way to verify that.

We could probably work around this problem a different way. Since this is the Angular decorator transform, we know a few things:

constructor parameters without @Inject must reference a type declaration that's also a value (and thus it's fine to "assume" when transpiling).

constructor parameters with @Inject don't need a type output at all, and we can unconditionally generate type: undefined.

For the 2nd case, can we unconditionally NOT generating undefined so that it’s possible to run tests and leave it up to Jest runtime to validate if that is valid runtime value?

For example:

import foo from ‘./foo’ would resolve in type as type: foo.<something>

And the same applies to import * as foo from ‘./foo’

If we set type: undefined, it will break all existing tests and it makes it not possible to run tests in transpilation mode because there will be error like Can't resolve all parameters for AppComponent: (?) which I observe when debugging this issue #963

Francesco-Borzi · 2024-09-04T14:23:08Z

hi, we are experiencing quite some performance issues when running unit tests using jest-preset-angular. I've been reading here and there on the web and bumped into this PR.

I wanted to first of all thank for your precious contributions, and I also wanted to ask if this PR is supposed to make tests run faster and how much is the expected impact.

ahnpnl · 2024-09-05T18:04:24Z

@Francesco-Borzi this PR opens up a possibility to improve both test speed and memory usage without breaking existing test suites.

At the moment, you can already use isolatedModules: true but there are caveats:

Some scenarios won’t work, which are demonstrated in this PR.
Ignore type checking. This can be easily enabled

Francesco-Borzi · 2024-09-05T21:31:16Z

@Francesco-Borzi this PR opens up a possibility to improve both test speed and memory usage without breaking existing test suites.

At the moment, you can already use isolatedModules: true but there are caveats:

Some scenarios won’t work, which are demonstrated in this PR.

Ignore type checking. This can be easily enabled

hi @ahnpnl , apart from the new compilation errors, I don't seem to get any performance improvement using isolatedModules: true

ahnpnl · 2024-09-05T21:50:06Z

Just to be sure, I mean

jest-preset-angular/examples/example-app-v18/jest-isolated.config.js

Line 13 in fd36bec

isolatedModules: true,

Often this option is mistaken with tsconfig option

Francesco-Borzi · 2024-09-05T22:28:54Z

@ahnpnl thanks for the clarification!

ahnpnl force-pushed the infra/add-transformer-unit-test branch from 4f9eb2a to b5b8e99 Compare July 22, 2024 21:50

dgp1130 reviewed Jul 23, 2024

View reviewed changes

ahnpnl force-pushed the infra/add-transformer-unit-test branch 5 times, most recently from a102b3a to e31377c Compare July 29, 2024 14:23

ahnpnl force-pushed the infra/add-transformer-unit-test branch from e31377c to c5a0f81 Compare August 2, 2024 09:23

ahnpnl changed the title ~~test: add unit tests for transpileModule with JIT transformers~~ test: add failing test cases with single file transpilation for JIT transformers Aug 2, 2024

ahnpnl mentioned this pull request Aug 2, 2024

Support single file Program for downlevel constructor transformer angular/angular#57237

Open

test: add failing test cases with single file transpilation

0db3190

ahnpnl force-pushed the infra/add-transformer-unit-test branch from c5a0f81 to 0db3190 Compare August 2, 2024 10:03

ahnpnl mentioned this pull request Aug 9, 2024

[Bug]: Slow jest test memory leak #2161

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add failing test cases with single file transpilation for JIT transformers #2615

test: add failing test cases with single file transpilation for JIT transformers #2615

ahnpnl commented Jul 22, 2024

ahnpnl commented Jul 22, 2024

dgp1130 left a comment

dgp1130 Jul 23, 2024

ahnpnl Jul 23, 2024 •

edited

Loading

dgp1130 Jul 24, 2024

dgp1130 Jul 23, 2024

ahnpnl Jul 23, 2024

dgp1130 Jul 24, 2024

dgp1130 Jul 23, 2024

ahnpnl Jul 23, 2024 •

edited

Loading

dgp1130 Jul 23, 2024

ahnpnl Jul 23, 2024 •

edited

Loading

dgp1130 Jul 24, 2024

ahnpnl Jul 29, 2024 •

edited

Loading

dgp1130 Aug 1, 2024

alxhub Aug 5, 2024

ahnpnl Aug 9, 2024

Francesco-Borzi commented Sep 4, 2024

ahnpnl commented Sep 5, 2024

Francesco-Borzi commented Sep 5, 2024

ahnpnl commented Sep 5, 2024

Francesco-Borzi commented Sep 5, 2024

test: add failing test cases with single file transpilation for JIT transformers #2615

Are you sure you want to change the base?

test: add failing test cases with single file transpilation for JIT transformers #2615

Conversation

ahnpnl commented Jul 22, 2024

Test plan

Does this PR introduce a breaking change?

Other information

ahnpnl commented Jul 22, 2024

dgp1130 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnpnl Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnpnl Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnpnl Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnpnl Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Francesco-Borzi commented Sep 4, 2024

ahnpnl commented Sep 5, 2024

Francesco-Borzi commented Sep 5, 2024

ahnpnl commented Sep 5, 2024

Francesco-Borzi commented Sep 5, 2024

ahnpnl Jul 23, 2024 •

edited

Loading

ahnpnl Jul 23, 2024 •

edited

Loading

ahnpnl Jul 23, 2024 •

edited

Loading

ahnpnl Jul 29, 2024 •

edited

Loading