Upgrade Angular build process and tests#590
Conversation
uldisrudzitis
added
Enhancement
dependencies
WalkthroughSwap Angular DevKit builders to @angular/build, move tsconfig app/spec to repository root and enable stricter TypeScript/Angular checks, remove src-level polyfills/test bootstrap and Karma config, update package.json test/tooling deps, add a lightweight component unit test and CI job for Angular tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CLI as npm / ng scripts
participant Builder as @angular/build
participant DevServer as Dev Server
participant TestRunner as Karma/@angular/build:karma
participant Browser as Browser
Dev->>CLI: ng serve / npm start
CLI->>Builder: invoke dev-server builder (browser: src/main.ts, polyfills: zone.js)
Builder->>DevServer: start serving assets
DevServer-->>Browser: deliver app
Dev->>CLI: ng test / npm test
CLI->>Builder: invoke karma builder (polyfills: zone.js/testing, tsConfig: tsconfig.spec.json)
Builder->>TestRunner: run tests (Jasmine + karma-coverage)
TestRunner-->>CLI: return test results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
editor/tsconfig.spec.json (1)
1-8: Limit spec compilation to spec files to reduce build time and noiseIncluding all source files in tests is unnecessary. Let TypeScript pull app files via imports from specs.
{ "extends": "./tsconfig.json", "compilerOptions": { "outDir": "./out-tsc/spec", - "types": ["jasmine"] + "types": ["jasmine"] }, - "include": ["src/**/*.ts"] + "include": ["src/**/*.spec.ts", "src/**/*.d.ts"] }Optional: add "node" to types if tests use Node globals (e.g., process, Buffer).
editor/angular.json (3)
64-70: Use hidden source maps in production (safer for Sentry uploads)Avoid serving source maps publicly while keeping them for Sentry.
- "sourceMap": true, + "sourceMap": { "hidden": true },
71-75: Disable optimization in development for faster rebuilds and better debuggingCurrent dev config has optimization enabled; turn it off.
- "development": { - "optimization": true, + "development": { + "optimization": false, "extractLicenses": false, "sourceMap": true }
96-103: Add ChromeHeadless to CI test targetPin Karma to run headless in CI by adding the
browsersoption:"test": { "builder": "@angular/build:karma", "options": { "polyfills": ["zone.js", "zone.js/testing"], "tsConfig": "tsconfig.spec.json", + "browsers": "ChromeHeadless", "inlineStyleLanguage": "scss", "assets": ["src/favicon.ico", "src/assets"], "styles": ["src/styles/styles.scss"] } }No references to
polyfills.tsorsrc/test.tsremain in the codebase.editor/tsconfig.json (1)
6-18: Consider turning on TS/Angular strictness and bundler resolution (staged)To catch more issues at build time and align with esbuild:
- Enable strict and strictTemplates (can be staged per sub-option).
- Use moduleResolution: "bundler" for modern ESM/package exports.
"compilerOptions": { "baseUrl": "./", "sourceMap": true, - "strict": false, - "noImplicitOverride": false, - "noPropertyAccessFromIndexSignature": false, - "noImplicitReturns": false, + "strict": true, + "noImplicitOverride": true, + "noPropertyAccessFromIndexSignature": true, + "noImplicitReturns": true, "noFallthroughCasesInSwitch": true, "skipLibCheck": true, "isolatedModules": true, "experimentalDecorators": true, "importHelpers": true, "useDefineForClassFields": false, "target": "ES2022", - "module": "preserve" + "module": "preserve", + "moduleResolution": "bundler" }, "angularCompilerOptions": { "enableI18nLegacyMessageIdFormat": false, "strictInjectionParameters": true, "strictInputAccessModifiers": true, "typeCheckHostBindings": true, - "strictTemplates": false + "strictTemplates": true },If this is too noisy right now, enable incrementally (e.g., first noImplicitOverride, then strictTemplates).
Also applies to: 19-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
editor/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
editor/angular.json(3 hunks)editor/angular.update.log.md(0 hunks)editor/package.json(1 hunks)editor/src/karma.conf.js(0 hunks)editor/src/polyfills.ts(0 hunks)editor/src/test.ts(0 hunks)editor/src/tsconfig.app.json(0 hunks)editor/src/tsconfig.spec.json(0 hunks)editor/tsconfig.app.json(1 hunks)editor/tsconfig.json(1 hunks)editor/tsconfig.spec.json(1 hunks)
💤 Files with no reviewable changes (6)
- editor/src/tsconfig.app.json
- editor/angular.update.log.md
- editor/src/polyfills.ts
- editor/src/tsconfig.spec.json
- editor/src/karma.conf.js
- editor/src/test.ts
🔇 Additional comments (2)
editor/tsconfig.app.json (1)
1-9: App/spec split looks correctIncludes app sources and excludes specs; empty types avoids Jasmine leakage. LGTM.
editor/package.json (1)
43-61: DevDependencies versions are compatible with Angular 20
All specified versions (TS ~5.8.3, Node 24.x, @angular/*@20.3.x) satisfy official Angular 20 compatibility requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
editor/angular.json (2)
49-55: Scripts/CommonJS and polyfills: verify builder compatibility
polyfills: ["zone.js"]is correct for apps. Ensurezone.jsis present in deps.allowedCommonJsDependencieswarnings/behavior differ with@angular/build(esbuild). Verify it’s still honored; if not, expect either no CJS warnings or different suppression.- Global
scriptsfromnode_moduleswork, but prefer module imports to avoid globals and improve tree-shaking.Refactor suggestion (if Twig supports ESM or side-effect import):
- "scripts": ["./node_modules/twig/twig.min.js"] + "scripts": []Then import once in your app bootstrap (e.g., main.ts or a dedicated module):
// main.ts import 'twig/twig.min.js';If Twig lacks ESM, keep the script but drop the leading "./" for consistency:
- "scripts": ["./node_modules/twig/twig.min.js"] + "scripts": ["node_modules/twig/twig.min.js"]
96-103: Karma builder migration: verify defaults and assets/styles inclusion
@angular/build:karmawith updated polyfills and tsconfig looks consistent. Sincekarma.conf.jsis removed, ensure defaults (browsers, reporters, coverage) meet CI needs. Includingassets/stylesunder test is atypical; keep only if tests rely on them to render components with global styles/assets.If not needed:
- "assets": ["src/favicon.ico", "src/assets"], - "styles": ["src/styles/styles.scss"] + "assets": [], + "styles": []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
editor/angular.json(3 hunks)
🔇 Additional comments (5)
editor/angular.json (5)
40-47: Double-check outputPath object and path traversal outside workspaceThe object form of
outputPathis typically used for SSR (browser/server splits). Here,base: "../engine/dist"escapes the workspace root; newer builders may block that. Confirm@angular/build:applicationaccepts:
- object shape without SSR,
- and an output directory outside the workspace.
If disallowed, switch to a workspace-internal path and move/copy artifacts in a follow-up step.
- "outputPath": { - "base": "../engine/dist", - "browser": "" - }, + "outputPath": "dist/engine",
72-75: Dev options naming parityDevelopment config uses
sourceMap: true. Confirm whether the new builder expectssourcemap. If mismatched, the option may be ignored or fail validation.If needed:
- "sourceMap": true + "sourcemap": true
79-86: Serve migration looks good
@angular/build:dev-serverwithbuildTarget: "berta:build"is correct for the new builder.
90-94: i18n extraction migration looks good
@angular/build:extract-i18nwithbuildTargetis aligned with the new tooling.
66-69: Remove legacynamedChunksand adjustsourceMap
@angular/build:applicationno longer supports the Webpack-eranamedChunksoption or the{ "hidden": true }shape undersourceMap. Replace both with the newsourcemapboolean (e.g.,sourcemap: false) or the proper schema for hidden source maps in your Angular CLI version.
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Caution The CodeRabbit agent failed during execution: Stream setup permanently failed: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/tests.yml (1)
12-12: Update checkout action to v4.Both jobs use
actions/checkout@v3, which is outdated. Update to@v4to ensure compatibility with the current GitHub Actions runner environment.Apply this diff:
- name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4Apply to both occurrences (lines 12 and 33).
Also applies to: 33-33
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.yml(2 hunks)WARP.md(1 hunks)editor/src/app/app.component.spec.ts(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/tests.yml
33-33: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
WARP.md
[grammar] ~51-~51: There might be a mistake here.
Context: ...dular architecture with inline templates - Outputs built files to ../engine/dist/...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Angular Editor Tests
🔇 Additional comments (3)
.github/workflows/tests.yml (1)
27-46: Angular tests job is well-configured.The new Angular tests job correctly sets up Node.js 20, uses npm caching for improved performance, and runs tests in headless Chrome mode—appropriate for CI environments.
editor/src/app/app.component.spec.ts (2)
4-25: Consider the limitations of testing a substitute component.While creating a
TestAppComponentis pragmatic given NGXS dependencies, this approach means you're not actually testing the realAppComponentbehavior, state management, or lifecycle. Consider whether integration tests with mocked NGXS stores or selective unit tests of specific AppComponent methods might provide better coverage in the future.
31-39: Test setup is appropriate.Using
NO_ERRORS_SCHEMAis a reasonable choice here to avoid having to declare or mock all child components (berta-header, berta-preview, etc.), keeping the test focused on the component structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
editor/src/app/app.component.spec.ts (1)
34-34: Consider using mock components instead of NO_ERRORS_SCHEMA.
NO_ERRORS_SCHEMAsuppresses all template errors, which can hide legitimate issues with child components. While it simplifies setup, it reduces test reliability.If the goal is just to ignore specific child components, consider creating mock components:
@Component({ selector: 'berta-header', template: '' }) class MockHeaderComponent {} @Component({ selector: 'berta-preview', template: '' }) class MockPreviewComponent {} @Component({ selector: 'berta-popup', template: '' }) class MockPopupComponent {} // Then in TestBed: declarations: [TestAppComponent, MockHeaderComponent, MockPreviewComponent, MockPopupComponent], // schemas: [NO_ERRORS_SCHEMA], // Remove thisThis approach is more explicit and catches typos or missing components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(2 hunks)editor/src/app/app.component.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yml
🔇 Additional comments (3)
editor/src/app/app.component.spec.ts (3)
1-6: Good documentation of the testing approach.The comment clearly explains why a simplified test component is used instead of the real AppComponent. The imports are appropriate for the testing approach.
7-25: Consider mocking NGXS dependencies instead of using a test double.While the test component approach is documented, it means the real
AppComponentis never tested. Over time, theTestAppComponentcould diverge from the actual implementation, reducing test value.Can NGXS dependencies be mocked using
provideMockStoreor similar utilities to test the realAppComponentinstead? This would provide more confidence that the actual component works as expected.
41-54: Tests look good; past review feedback has been addressed.The test naming issue from the previous review has been resolved. The test on line 45 is now appropriately named "should render main template structure" which accurately describes what it verifies.
The three tests provide basic coverage:
- Component instantiation
- Template structure rendering
- Property initialization
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
editor/src/app/app.component.spec.ts (3)
4-25: Pragmatic approach with room for enhancement.The comment clearly explains the rationale for using a simplified test component, which is reasonable given NGXS dependency complexity. The TestAppComponent structure mirrors the real component adequately for lightweight testing.
One consideration: the template includes
<router-outlet>(line 13) but no router setup is provided. This works only because NO_ERRORS_SCHEMA suppresses the error. If you later want to test routing behavior, you'd need to addRouterTestingModuleto the imports.Consider for future enhancements:
// In TestBed.configureTestingModule imports: [RouterTestingModule],
31-35: Consider shallow component testing as an alternative to NO_ERRORS_SCHEMA.The current setup works and is pragmatic for a lightweight test. However, NO_ERRORS_SCHEMA can mask genuine template errors. As an optional improvement, consider using component stubs instead:
// Create stub components @Component({selector: 'berta-header', template: ''}) class BertaHeaderStub {} @Component({selector: 'berta-preview', template: ''}) class BertaPreviewStub {} @Component({selector: 'berta-popup', template: ''}) class BertaPopupStub {} // Then in TestBed: await TestBed.configureTestingModule({ declarations: [TestAppComponent, BertaHeaderStub, BertaPreviewStub, BertaPopupStub], imports: [RouterTestingModule], // Remove NO_ERRORS_SCHEMA }).compileComponents();This approach catches more errors while still keeping tests lightweight. However, given the PR's focus on build infrastructure, the current NO_ERRORS_SCHEMA approach is acceptable.
41-54: Tests are functional and past issues have been addressed.Good work addressing the previous review comments:
- ✅ Test name "should render main template structure" now accurately reflects what's being tested
- ✅ Describe block correctly named "TestAppComponent"
The tests provide basic coverage appropriate for a lightweight test suite. As optional enhancements, you could add more comprehensive template structure verification:
it('should render main template structure', () => { fixture.detectChanges(); const compiled = fixture.nativeElement as HTMLElement; expect(compiled.querySelector('main')).toBeTruthy(); // Optional: verify other key template elements expect(compiled.querySelector('berta-header')).toBeTruthy(); expect(compiled.querySelector('berta-preview')).toBeTruthy(); expect(compiled.querySelector('berta-popup')).toBeTruthy(); expect(compiled.querySelector('router-outlet')).toBeTruthy(); });However, given the PR's focus on upgrading the build process and test infrastructure, the current minimal test coverage is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
editor/src/app/app.component.spec.ts(1 hunks)
🔇 Additional comments (1)
editor/src/app/app.component.spec.ts (1)
1-2: LGTM!The imports are appropriate for this lightweight test setup. NO_ERRORS_SCHEMA is imported for pragmatic reasons explained in the file comments.
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
Documentation