test: add aot option to karma #28854

yjaaidi · 2024-11-13T21:21:08Z

PR Checklist

Please check to confirm your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular-cli/blob/main/CONTRIBUTING.md#-commit-message-guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Tests run in JIT only.

What is the new behavior?

Tests can run in AOT, using the new aot option.

Does this PR introduce a breaking change?

Yes
No

Other information

yjaaidi · 2024-11-13T21:38:58Z

packages/angular_devkit/build_angular/src/builders/karma/tests/options/aot_spec.ts

+              })
+              class Hello {}
+
+              expect((Hello as any).ɵcmp.template.toString()).not.toContain('jit');


This is fragile and sensitive to any structural changes in JIT. It could easily cause false negatives.
What would be a better way to achieve this detection?

I think as long as we have a second test case that tests the reverse, it's fine..? That way we can make sure that "jit" appears in one branch and doesn't appear in the other, with aot: true vs aot: false as the only difference. That seems like an okay state to be in for now.

I've added the jit test to make sure jit symbols are there.

yjaaidi · 2024-11-13T21:40:33Z

packages/angular_devkit/build_angular/src/builders/karma/tests/options/aot_spec.ts

+        ...BASE_OPTIONS,
+        aot: true,
+        /** Cf. {@link ../builder-mode_spec.ts} */
+        polyfills: ['zone.js', '@angular/localize/init', 'zone.js/testing'],


Should we factorize this in BASE_OPTIONS? or create a BASE_POLYFILLS? or an object mother: optionsMother.base().withPolyfills()?

describeKarmaBuilder already runs the builder in both modes automatically. You shouldn't need any override but aot: true in here if everything works as expected. The builder mode spec is special because it explicitly tests that the mode can be changed by explicitly setting this. But it seems wasteful to rerun this test case twice in application builder mode (which I think is what's currently happening with this file).

jkrems · 2024-11-15T18:28:51Z

Sorry for the lack of review so far but I just want to quickly say that I'm really excited to see this option added. aot all the things! 🎉 - goes back to working through final fixes for the rc candidate

yjaaidi · 2024-11-15T19:01:47Z

No rush at all!

jkrems

LGTM - sorry for the late review!

jkrems · 2024-12-05T22:00:03Z

packages/angular_devkit/build_angular/src/builders/karma/tests/options/aot_spec.ts

+        ...BASE_OPTIONS,
+        aot: true,
+        /** Cf. {@link ../builder-mode_spec.ts} */
+        polyfills: ['zone.js', '@angular/localize/init', 'zone.js/testing'],


describeKarmaBuilder already runs the builder in both modes automatically. You shouldn't need any override but aot: true in here if everything works as expected. The builder mode spec is special because it explicitly tests that the mode can be changed by explicitly setting this. But it seems wasteful to rerun this test case twice in application builder mode (which I think is what's currently happening with this file).

jkrems · 2024-12-05T22:01:14Z

packages/angular_devkit/build_angular/src/builders/karma/tests/options/aot_spec.ts

+import { BASE_OPTIONS, KARMA_BUILDER_INFO, describeKarmaBuilder } from '../setup';
+import { BuilderMode } from '../../schema';
+
+describeKarmaBuilder(execute, KARMA_BUILDER_INFO, (harness, setupTarget) => {


If you want to restrict this to application builder mode only, you might need to add the 3rd argument for this callback (isApplicationBuilder) and skip/disable the test if that boolean is false.

jkrems · 2024-12-05T22:08:49Z

packages/angular_devkit/build_angular/src/builders/karma/tests/options/aot_spec.ts

+              })
+              class Hello {}
+
+              expect((Hello as any).ɵcmp.template.toString()).not.toContain('jit');


I think as long as we have a second test case that tests the reverse, it's fine..? That way we can make sure that "jit" appears in one branch and doesn't appear in the other, with aot: true vs aot: false as the only difference. That seems like an okay state to be in for now.

yjaaidi · 2024-12-07T10:28:24Z

Thx! No problem at all.

jkrems · 2025-02-04T15:41:30Z

Sorry for the delay here. Approved the change but there's a tiny conflict and I don't think I can resolve it without push access to the branch. Can you undo my merge commit (e.g. git reset --hard 48d0a96b53f9464f334248999408aa1c3c597bbf) and then rebase?

jkrems · 2025-02-12T17:35:19Z

Thanks for the updated test! Can you rebase instead of doing the merge?

Also, given that the base version landed (#29608), I think the commit message could be reworded to be test:.

…abled by default

yjaaidi · 2025-02-25T09:31:42Z

Thanks for the updated test! Can you rebase instead of doing the merge?

Also, given that the base version landed (#29608), I think the commit message could be reworded to be test:.

Hey @jkrems, sorry for the delay.
I've updated the test, and squash rebased.

jkrems · 2025-02-25T15:31:48Z

packages/angular_devkit/build_angular/src/builders/karma/tests/options/aot_spec.ts

        'src/aot.spec.ts': `
          import { Component } from '@angular/core';
-
+          


nit: Here and in line 44 is some trailing whitespace.

angular-automatic-lock-bot · 2025-03-28T00:12:22Z

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-robot bot added the detected: feature PR contains a feature commit label Nov 13, 2024

yjaaidi commented Nov 13, 2024

View reviewed changes

yjaaidi mentioned this pull request Nov 13, 2024

feat: add aot option to jest #28855

Merged

13 tasks

yjaaidi force-pushed the feat/add-aot-option-to-karma branch from 38ac095 to 48d0a96 Compare November 14, 2024 00:42

angular-robot bot added the area: @angular-devkit/build-angular label Nov 14, 2024

clydin added the target: minor This PR is targeted for the next minor release label Nov 15, 2024

jkrems reviewed Dec 5, 2024

View reviewed changes

yjaaidi mentioned this pull request Dec 9, 2024

Feature Request: Coverage for template files #25563

Open

alan-agius4 added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 9, 2025

jkrems self-assigned this Jan 28, 2025

jkrems approved these changes Feb 4, 2025

View reviewed changes

yjaaidi force-pushed the feat/add-aot-option-to-karma branch from 9a723a5 to e46d31e Compare February 12, 2025 17:16

angular-robot bot requested a review from jkrems February 12, 2025 17:17

test(@angular-devkit/build-angular): add test to make sure aot is dis…

2322240

…abled by default

yjaaidi mentioned this pull request Feb 25, 2025

test(@angular-devkit/build-angular): add test to make sure aot is disabled by default #29704

Closed

13 tasks

yjaaidi force-pushed the feat/add-aot-option-to-karma branch from 83c94a6 to 2322240 Compare February 25, 2025 09:30

angular-robot bot removed the detected: feature PR contains a feature commit label Feb 25, 2025

yjaaidi changed the title ~~feat: add aot option to karma~~ test: add aot option to karma Feb 25, 2025

jkrems approved these changes Feb 25, 2025

View reviewed changes

jkrems added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 25, 2025

jkrems merged commit 9c1c43b into angular:main Feb 25, 2025
35 checks passed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 28, 2025

		'src/aot.spec.ts': `
		import { Component } from '@angular/core';

test: add aot option to karma #28854

test: add aot option to karma #28854

Uh oh!

Conversation

yjaaidi commented Nov 13, 2024

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkrems commented Nov 15, 2024

Uh oh!

yjaaidi commented Nov 15, 2024

Uh oh!

jkrems left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjaaidi commented Dec 7, 2024

Uh oh!

jkrems commented Feb 4, 2025

Uh oh!

jkrems commented Feb 12, 2025

Uh oh!

yjaaidi commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Mar 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants