[ZEPPELIN-6567] Give the Angular shell a self-contained unit test setup - #5401
[ZEPPELIN-6567] Give the Angular shell a self-contained unit test setup#5401miinhho wants to merge 5 commits into
Conversation
voidmatcha
left a comment
There was a problem hiding this comment.
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.
| const { defineConfig } = require('vitest/config'); | ||
|
|
||
| module.exports = defineConfig({ | ||
| test: { | ||
| environment: 'jsdom', | ||
| include: ['src/**/*.spec.ts'], | ||
| setupFiles: ['./test/test-setup.ts'] | ||
| } | ||
| }); |
There was a problem hiding this comment.
Now that vitest is local dependency, we could convert this into ESM way.
| 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
left a comment
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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:
| await Promise.resolve(); | |
| await vi.waitFor(() => expect(zoneStates).toHaveLength(1)); |
|
I used |
voidmatcha
left a comment
There was a problem hiding this comment.
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
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-reactforVitest/jsdom. That meant the shell test command crossed a package boundary, the Vitest config could not normally importvitest/config, and the spec tsconfig was not usable for real type checking.This PR adds
Vitest/jsdomto the Angular package itself, adds a shell-only Vitest config and spec tsconfig, and wirestest:shellinto 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:
Questions: