Skip to content

[ZEPPELIN-6567] Give the Angular shell a self-contained unit test setup - #5401

Open
miinhho wants to merge 5 commits into
apache:masterfrom
miinhho:self-contained-angular-shell-test-setup
Open

[ZEPPELIN-6567] Give the Angular shell a self-contained unit test setup#5401
miinhho wants to merge 5 commits into
apache:masterfrom
miinhho:self-contained-angular-shell-test-setup

Conversation

@miinhho

@miinhho miinhho commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

What is this PR for?

This PR gives zeppelin-web-angular a self-contained unit test setup for Angular shell code.

Previously, the shell unit test path depended on projects/zeppelin-react for Vitest/jsdom. That meant the shell test command crossed a package boundary, the Vitest config could not normally import vitest/config, and the spec tsconfig was not usable for real type checking.

This PR adds Vitest/jsdom to the Angular package itself, adds a shell-only Vitest config and spec tsconfig, and wires test:shell into the existing npm run lint path so it runs through the Maven/GitHub test phase.

It also keeps the Vitest setup file outside src, so it is not pulled into production Angular compilation.

What type of PR is it?

Improvement

What is the Jira issue?

How should this be tested?

Run from zeppelin-web-angular:

npm run test:shell

Questions:

  • Does the license files need to update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@voidmatcha voidmatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Cutting the dependency on projects/zeppelin-react is the important part of this PR, and that lands cleanly. Wiring it into a path CI already runs means it cannot quietly rot.

One thing I would like changed. test:shell runs inside lint, so a failing test reports as a lint failure, and the script gets slower as specs accumulate.

zeppelin-web-angular/pom.xml:110 already binds npm run lint to the test phase, so a sibling execution in the same shape covers it. Same phase, so nothing changes about when it runs.

<execution>
  <id>npm test shell</id>
  <goals>
    <goal>npm</goal>
  </goals>
  <phase>test</phase>
  <configuration>
    <arguments>run test:shell</arguments>
  </configuration>
</execution>

Then drop && npm run test:shell from the lint script, and while you are there, README.md:56 still says to run ng test via Karma. There is no test architect in angular.json, so that instruction does not work today, and this PR is what finally gives it a real answer. All three are on this branch if it helps:
https://github.com/voidmatcha/zeppelin/tree/ZEPPELIN-6567-test-phase

Separately, the setup cannot compile Angular decorators yet, so component and directive specs are still out of reach. Filed that as ZEPPELIN-6637.

Comment on lines +13 to +21
const { defineConfig } = require('vitest/config');

module.exports = defineConfig({
test: {
environment: 'jsdom',
include: ['src/**/*.spec.ts'],
setupFiles: ['./test/test-setup.ts']
}
});

@tbonelee tbonelee Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now that vitest is local dependency, we could convert this into ESM way.

Suggested change
const { defineConfig } = require('vitest/config');
module.exports = defineConfig({
test: {
environment: 'jsdom',
include: ['src/**/*.spec.ts'],
setupFiles: ['./test/test-setup.ts']
}
});
import { defineConfig } from 'vitest/config';
export default defineConfig({
test: {
environment: 'jsdom',
include: ['src/**/*.spec.ts'],
setupFiles: ['./test/test-setup.ts']
}
});

@tbonelee tbonelee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One inline comment below, plus the require() suggestion I left earlier on vitest.shell.config.ts.

directive.ngOnChanges({
module: new SimpleChange(undefined, directive.module, true)
});
await Promise.resolve();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

await Promise.resolve() only holds while startLoad() awaits exactly once. I tried adding one more await there and this failed with expected [] to deeply equal [ false ], which reads as a broken assertion rather than a timing issue. Waiting on the effect is sturdier:

Suggested change
await Promise.resolve();
await vi.waitFor(() => expect(zoneStates).toHaveLength(1));

@miinhho

miinhho commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

I used .mts in vitest.shell.config so the Vitest config is explicitly loaded as ESM. With .ts, Vite warns about ESM syntax in a CommonJS-loaded config.

@voidmatcha voidmatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Removing runOutsideAngular from startLoad leaves the suite green, so the zone assertion cannot currently fail. zone.js does not patch the native await vitest emits, but spying on the call catches it. Likely useful when you get to ZEPPELIN-6565.

.mts not matching **/*.ts in eslint.config.js:41 is in there too.

Both applied on this branch if you want them.
https://github.com/voidmatcha/zeppelin/tree/ZEPPELIN-6567-followups

@voidmatcha voidmatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

@miinhho
miinhho requested a review from tbonelee August 12, 2026 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants