fix(compiler): properly compile DI factories when coverage reporting is enabled by JoostK · Pull Request #37811 · angular/angular

JoostK · 2020-06-28T21:35:49Z

When running tests with code coverage using Istanbul, the code is
instrumented with coverage reporting statements. These statements are
also inserted into synthesized constructors, preventing Angular from
properly recognizing them as synthesized constructor.

This commit extends the logic to detect synthesized constructors by
first canonicalizing the constructor's code, which strips out Istanbul's
instrumentation logic.

The tests have been extended with an input file that is being
instrumented using the babel-plugin-istanbul for both ES2015 and ES5
targets, in order to verify that the approach works for real-world
usages.

Fixes #31337

sviat9440 · 2020-09-15T14:41:03Z

Any news?

JoostK · 2020-10-25T13:54:34Z

packages/core/src/reflection/reflection_capabilities.ts

Note: this avoids pulling in the Istanbul related canonicalization logic into production logic, with the downside that testing environments need to configure it correctly. This PR does that for TestBed, but other situations are not covered. Should we just pull in the canonicalization logic here instead? That seems a bit simpler, but I'd love some feedback here.

klemenoslaj · 2021-05-12T08:38:25Z

Any news on this?

JoostK · 2021-05-12T08:48:22Z

@klemenoslaj I haven't had cycles to work on this due to v12 priorities, but I was planning to pick up on this sometime soon.

klemenoslaj · 2021-05-12T08:51:18Z

@klemenoslaj I haven't had cycles to work on this due to v12 priorities, but I was planning to pick up on this sometime soon.

Thanks for a super fast reply @JoostK.
I am now setting target to ES5, but just in the tsconfig.spec.json. Is that a valid workaround?

JoostK · 2021-05-12T09:02:23Z

@klemenoslaj I haven't had cycles to work on this due to v12 priorities, but I was planning to pick up on this sometime soon.

Thanks for a super fast reply @JoostK.
I am now setting target to ES5, but just in the tsconfig.spec.json. Is that a valid workaround?

AFAIK the problem would also occur in ES5, as the code coverage statements cause Angular to consider a constructor as manually written instead of being just a delegated constructor to the base type.

A workaround may be to always write the constructor, including all parameters and a super call in any of your own classes. This doesn't help for libraries but that shouldn't be a problem if they are AOT compiled by ngcc (and not recompiled by TestBed using override* API). This does have some bundle size impact thought.

klemenoslaj · 2021-05-12T11:53:09Z

@klemenoslaj I haven't had cycles to work on this due to v12 priorities, but I was planning to pick up on this sometime soon.

Thanks for a super fast reply @JoostK.
I am now setting target to ES5, but just in the tsconfig.spec.json. Is that a valid workaround?

AFAIK the problem would also occur in ES5, as the code coverage statements cause Angular to consider a constructor as manually written instead of being just a delegated constructor to the base type.

A workaround may be to always write the constructor, including all parameters and a super call in any of your own classes. This doesn't help for libraries but that shouldn't be a problem if they are AOT compiled by ngcc (and not recompiled by TestBed using override* API). This does have some bundle size impact thought.

Interestingly, I just tested with and without es5 setup. Without it fails, with it works fine. 🤔

…is enabled When running tests with code coverage using Istanbul, the code is instrumented with coverage reporting statements. These statements are also inserted into synthesized constructors, preventing Angular from properly recognizing them as synthesized constructor. This commit extends the logic to detect synthesized constructors by first canonicalizing the constructor's code, which strips out Istanbul's instrumentation logic. The tests have been extended with an input file that is being instrumented using the `babel-plugin-istanbul` for both ES2015 and ES5 targets, in order to verify that the approach works for real-world usages. Fixes angular#31337

JoostK · 2021-10-31T19:49:57Z

@klemenoslaj I haven't had cycles to work on this due to v12 priorities, but I was planning to pick up on this sometime soon.

Thanks for a super fast reply @JoostK.
I am now setting target to ES5, but just in the tsconfig.spec.json. Is that a valid workaround?

AFAIK the problem would also occur in ES5, as the code coverage statements cause Angular to consider a constructor as manually written instead of being just a delegated constructor to the base type.
A workaround may be to always write the constructor, including all parameters and a super call in any of your own classes. This doesn't help for libraries but that shouldn't be a problem if they are AOT compiled by ngcc (and not recompiled by TestBed using override* API). This does have some bundle size impact thought.

Interestingly, I just tested with and without es5 setup. Without it fails, with it works fine. 🤔

I have revised the PR a bit and replaced the integration test using the CLI with manually instrumenting code using babel-plugin-istanbul, which made it far easier to exercise the logic for both ES2015 and ES5. Surprisingly to me, the ES5 test passes even without the changes in this PR; so that confirms your findings for ES5. The ES5's regex is probably more lenient, allowing it to work as desired even with instrumentation instructions.

petebacondarwin

LGTM - a couple of suggestions.

packages/core/src/reflection/istanbul_canonicalizer.ts

packages/core/test/BUILD.bazel

petebacondarwin · 2021-10-31T19:59:07Z

BTW - I assume that core/reflections code does not normally appear in AOT built production ivy code?

JoostK · 2021-10-31T20:06:15Z

BTW - I assume that core/reflections code does not normally appear in AOT built production ivy code?

Indeed, it shouldn't. The ReflectionCapabilities should only be pulled in when using the JIT compiler.

…orting is enabled

JoostK · 2021-11-05T08:24:54Z

Converting back to draft as there may be an alternative fix that would be superior, if it works.

JoostK · 2022-01-21T21:13:14Z

Closing as superseded by #44732.

angular-automatic-lock-bot · 2022-02-21T16:06:36Z

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.}

JoostK added type: bug/fix state: WIP freq1: low severity3: broken area: core Issues related to the framework runtime workaround2: non-obvious target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler core: di labels Jun 28, 2020

ngbot bot modified the milestone: needsTriage Jun 28, 2020

googlebot added the cla: yes label Jun 28, 2020

JoostK mentioned this pull request Jun 28, 2020

Dependency injection and class inheritance doesn't work in Angular 8 library test #31337

Closed

jelbourn removed the severity3: broken label Oct 1, 2020

JoostK force-pushed the istanbul-instrumentation-ctor branch 3 times, most recently from 6070724 to 20c4d98 Compare October 25, 2020 13:25

JoostK added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Oct 25, 2020

JoostK marked this pull request as ready for review October 25, 2020 13:50

pullapprove bot requested review from IgorMinar and atscott October 25, 2020 13:50

JoostK commented Oct 25, 2020

View reviewed changes

JoostK removed the action: review The PR is still awaiting reviews from at least one requested reviewer label May 4, 2021

ngbot bot removed this from the needsTriage milestone May 4, 2021

ngbot bot added this to the Backlog milestone May 4, 2021

JoostK force-pushed the istanbul-instrumentation-ctor branch 2 times, most recently from 4bfc814 to c0db81c Compare October 31, 2021 11:13

JoostK force-pushed the istanbul-instrumentation-ctor branch from c0db81c to 9cc5fca Compare October 31, 2021 15:12

JoostK requested a review from petebacondarwin October 31, 2021 15:31

JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 31, 2021

petebacondarwin approved these changes Oct 31, 2021

View reviewed changes

packages/core/src/reflection/istanbul_canonicalizer.ts Outdated Show resolved Hide resolved

packages/core/test/BUILD.bazel Show resolved Hide resolved

fixup! fix(compiler): properly compile DI factories when coverage rep…

34ac61b

…orting is enabled

devversion self-requested a review November 5, 2021 07:12

JoostK added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 5, 2021

JoostK marked this pull request as draft November 5, 2021 08:24

devversion removed their request for review November 5, 2021 10:23

JoostK mentioned this pull request Nov 5, 2021

fix(compiler): use base class parameters for any non-decorated class … #44092

Closed

JoostK mentioned this pull request Jan 18, 2022

fix(compiler): properly compile DI factories when coverage reporting is enabled #44732

Closed

JoostK closed this Jan 21, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 21, 2022

Conversation

JoostK commented Jun 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sviat9440 commented Sep 15, 2020

Uh oh!

JoostK Oct 25, 2020

Choose a reason for hiding this comment

Uh oh!

klemenoslaj commented May 12, 2021

Uh oh!

JoostK commented May 12, 2021

Uh oh!

klemenoslaj commented May 12, 2021

Uh oh!

JoostK commented May 12, 2021

Uh oh!

klemenoslaj commented May 12, 2021

Uh oh!

JoostK commented Oct 31, 2021

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

petebacondarwin commented Oct 31, 2021

Uh oh!

JoostK commented Oct 31, 2021

Uh oh!

JoostK commented Nov 5, 2021

Uh oh!

JoostK commented Jan 21, 2022

Uh oh!

angular-automatic-lock-bot bot commented Feb 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JoostK commented Jun 28, 2020 •

edited

Loading