This is an automated email from the ASF dual-hosted git repository.
voidmatcha pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zeppelin.git
The following commit(s) were added to refs/heads/master by this push:
new 6a5ed44594 [ZEPPELIN-6724] Add a convention for frontend type tests
6a5ed44594 is described below
commit 6a5ed44594e88b22140e9159e6b96589fccf4e06
Author: ChanHo Lee <[email protected]>
AuthorDate: Sat Oct 3 11:05:08 2026 +0900
[ZEPPELIN-6724] Add a convention for frontend type tests
### What is this PR for?
Type tests in the frontend currently come in two layouts: assertions inside
an SDK spec (ZEPPELIN-6664) and a separate `type-tests/` directory
(ZEPPELIN-6663). This PR settles on the first and documents it in
`zeppelin-web-angular/AGENTS.md`, so the next type test has one place to go.
**The convention**
- Type assertions (`expectTypeOf`, `assertType`, `<at>ts-expect-error`) go
in ordinary `.spec.ts` files, beside the interface they pin.
- They count only where `tsc` checks the spec: today, the zeppelin-sdk and
notebook-core specs. Lint now accepts a type-only test there and still rejects
one elsewhere.
- To show a value is accepted, pass it to `assertType<T>(...)`. Prefer an
exact matcher to `<at>ts-expect-error`, which any error on the next line
satisfies.
**Why ordinary specs**
- Type assertions are erased before Vitest runs a spec, so only `tsc`
checks them. The two spec programs above already have a `tsc` pass in Maven
(`typecheck:sdk-contracts`, `typecheck:notebook-core`), so a type test there
needs no extra tsconfig, script or runner.
- The usual alternative is separate `*.test-d.ts` files checked by `vitest
--typecheck`. It reports failures per test, but it is still experimental and
reports a file outside its tsconfig as passed
([vitest#7988](https://github.com/vitest-dev/vitest/issues/7988)), so a type
test could go unchecked without anyone noticing. Per-test output is not worth
that risk or a second checker.
**Commits** (each passes typecheck, lint and tests on its own)
1. `eslint.config.js`: accept type assertions in the two typechecked spec
programs.
2. Move the `CompletionItem` type test into `completion-item.spec.ts`,
using exact matchers instead of `<at>ts-expect-error`.
3. Drop the now-empty `type-tests/` include from the SDK spec tsconfig.
4. List `typecheck:sdk-contracts` in AGENTS.md. Maven already ran it.
5. Document the convention.
6. Replace runtime `expect`s on typed literals in the SDK specs with
`assertType`. `tsc` already checked those literals; `assertType` keeps that
check, excess properties included, without a variable that needs a use.
### What type of PR is it?
Improvement
### Todos
* [x] Accept type-only tests in typechecked spec programs
* [x] Move the `CompletionItem` type test into an SDK spec
* [x] Document the convention in `zeppelin-web-angular/AGENTS.md`
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6724
### How should this be tested?
```bash
cd zeppelin-web-angular
npm run typecheck:sdk-contracts
npm run typecheck:notebook-core
npm run test:shell
npx eslint projects/zeppelin-sdk projects/zeppelin-notebook-core
test/notebook-core
```
- To see the lint boundary, put a test containing only `expectTypeOf` under
`src/`: `vitest/expect-expect` reports it. The same test under
`projects/zeppelin-sdk/src` passes.
- To see the type test fail, make `CompletionItem.name` optional:
`typecheck:sdk-contracts` reports `completion-item.spec.ts`.
### Screenshots (if appropriate)
N/A
### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? Yes, `zeppelin-web-angular/AGENTS.md` is
updated in this PR
Closes #5520 from tbonelee/ZEPPELIN-6724.
Signed-off-by: YONGJAE LEE <[email protected]>
---
zeppelin-web-angular/AGENTS.md | 25 ++++++++++++++++++++--
zeppelin-web-angular/eslint.config.js | 5 +++++
.../src/interfaces/completion-item.spec.ts | 24 +++++++++++++++++++++
.../message-data-type-map.interface.spec.ts | 8 +++----
.../src/interfaces/notebook-wire-fields.spec.ts | 13 +++++------
.../projects/zeppelin-sdk/tsconfig.spec.json | 2 +-
.../type-tests/completion-item-meta.ts | 24 ---------------------
7 files changed, 61 insertions(+), 40 deletions(-)
diff --git a/zeppelin-web-angular/AGENTS.md b/zeppelin-web-angular/AGENTS.md
index 29aab36887..27a29f4785 100644
--- a/zeppelin-web-angular/AGENTS.md
+++ b/zeppelin-web-angular/AGENTS.md
@@ -39,8 +39,9 @@ The repository root `AGENTS.md` asks every change to include
unit tests. This fi
| `npm run test:shell -- foo.spec.ts` | Run one file |
| `npm run test:notebook-core` | Run the dedicated notebook-core Node suite |
| `npm run typecheck:notebook-core` | Check core source and specs, rebuild the
package, and check the React type-only contract against built declarations and
the same core source |
+| `npm run typecheck:sdk-contracts` | Check the `zeppelin-sdk` specs,
including their [type assertions](#type-assertions) |
-`test:shell`, `test:notebook-core`, and `typecheck:notebook-core` are bound to
the Maven `test` phase (`pom.xml`), so a spec added here starts running in CI
the day it merges. It does not run where you would expect. `frontend.yml`
builds this module with `-DskipTests`, which frontend-maven-plugin honours by
skipping `test`-phase executions, so the run that counts is `mvnw verify
-Pweb-e2e` inside the `run-playwright-e2e-tests` job. A failing spec surfaces
there, under an e2e job name. Gi [...]
+`test:shell`, `test:notebook-core`, `typecheck:notebook-core`, and
`typecheck:sdk-contracts` are bound to the Maven `test` phase (`pom.xml`), so a
spec added here starts running in CI the day it merges. It does not run where
you would expect. `frontend.yml` builds this module with `-DskipTests`, which
frontend-maven-plugin honours by skipping `test`-phase executions, so the run
that counts is `mvnw verify -Pweb-e2e` inside the `run-playwright-e2e-tests`
job. A failing spec surfaces there [...]
## Where a test belongs
@@ -89,6 +90,26 @@ A spec with no assertion, or one whose assertion sits inside
an `if`, passes by
The e2e suite gets the same protection from `eslint-plugin-playwright`.
+## Type assertions
+
+`expectTypeOf`, `assertType` and `@ts-expect-error` are erased before a spec
runs, so Vitest passes them whatever they say. Only a `tsc` pass over the spec
checks them, and only two spec programs have one in Maven:
+
+| Specs | Checked by |
+| --- | --- |
+| `projects/zeppelin-sdk` | `typecheck:sdk-contracts` |
+| `projects/zeppelin-notebook-core`, `test/notebook-core` |
`typecheck:notebook-core` |
+| `src/`, the rest of `test/`, `projects/zeppelin-visualization` | nothing yet
|
+| `projects/zeppelin-react` | nothing yet
([ZEPPELIN-6566](https://issues.apache.org/jira/browse/ZEPPELIN-6566)) |
+
+Lint encodes the table: `vitest/expect-expect` accepts a type assertion as a
test's only assertion in the first two rows and rejects it elsewhere. A type
assertion in an unchecked spec, even beside an `expect`, cannot fail. When a
program gains a `tsc` pass in Maven, add its glob to the `settings: { vitest: {
typecheck: true } }` block in `eslint.config.js`, or set the same in
`projects/zeppelin-react/eslint.config.js`.
+
+- Put type assertions in an ordinary `.spec.ts`, never a separate type-test
file or directory. One SDK interface file declares many unrelated types, so a
type contract spec sits beside that file and is named for the contract it pins:
`notebook-wire-fields.spec.ts`, `completion-item.spec.ts`.
+- To show a value is accepted, pass it to `assertType<T>(...)`. A typed
variable then needs a use, and a runtime `expect` on a literal only restates
the literal.
+- Prefer an exact matcher to `@ts-expect-error`.
`expectTypeOf<CompletionItem>().toHaveProperty('name').toEqualTypeOf<string>()`
fails when `name` becomes optional; `@ts-expect-error` is satisfied by any
error on the next line, a typo included. Keep the directive for what no matcher
can say, such as assigning to a `readonly` member
(`host-remote-contract.spec.ts`), with one statement under it and the expected
failure after it.
+- A type assertion pins what the SDK declares, not what the server sends.
Payload shapes are evidenced by the server code that builds them, and later by
the captured-traffic contract specs described above.
+
+`vitest --typecheck` with `*.test-d.ts` files is not used: it is still
experimental, and it reports a file its tsconfig does not include as passed
([vitest#7988](https://github.com/vitest-dev/vitest/issues/7988)).
[TSTyche](https://tstyche.org) checks the message after `@ts-expect-error` and
has no such gap, but it is a second runner for a handful of files. Revisit it
if must-not-compile contracts multiply or a swallowed error is found.
+
## monaco-editor and path aliases in specs
`vitest.shell.config.mts` mirrors the `paths` block in `tsconfig.base.json`,
which Vite does not read; add an alias there when you add a path. Eleven files
under `src/` import `monaco-editor`, two behind the `@zeppelin/services`
barrel, so a spec that reaches the editor or notebook area loads it: a few
seconds on first import and a `marked.umd.js.map` sourcemap warning, both
monaco's, not ours. Mock it with `vi.mock('monaco-editor', ...)` when the spec
only needs to assert the editor was [...]
@@ -144,4 +165,4 @@ This is a different measurement from
`e2e/reporter.coverage.ts`, which counts an
3. Import from `vitest` (`describe`, `expect`, `it`), not from Jasmine or Jest.
Check the target has callers before you invest in it.
`get-keyword-positions.spec.ts` is a worked example of a function that turned
out to have none.
4. Construct the class directly unless the behavior depends on Angular wiring;
use `TestBed` when it does.
-5. Run `npm run test:shell` for shell/SDK/visualization changes. For
notebook-core changes, run `npm run test:notebook-core` and `npm run
typecheck:notebook-core`. Confirm the relevant checks pass before opening a PR.
+5. Run `npm run test:shell` for shell/SDK/visualization changes, plus `npm run
typecheck:sdk-contracts` for SDK changes. For notebook-core changes, run `npm
run test:notebook-core` and `npm run typecheck:notebook-core`. Confirm the
relevant checks pass before opening a PR.
diff --git a/zeppelin-web-angular/eslint.config.js
b/zeppelin-web-angular/eslint.config.js
index 239a119789..7b7c9d5bde 100644
--- a/zeppelin-web-angular/eslint.config.js
+++ b/zeppelin-web-angular/eslint.config.js
@@ -220,6 +220,11 @@ module.exports = tseslint.config(
'vitest/no-focused-tests': 'error'
}
},
+ {
+ // Only specs a Maven tsc pass compiles; anywhere else a type-only test
cannot fail.
+ files: ['projects/zeppelin-{notebook-core,sdk}/**/*.spec.ts',
'test/notebook-core/**/*.spec.ts'],
+ settings: { vitest: { typecheck: true } }
+ },
{
// The shell test setup intentionally loads Zone.js for its side effects.
files: ['test/test-setup.ts'],
diff --git
a/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/completion-item.spec.ts
b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/completion-item.spec.ts
new file mode 100644
index 0000000000..b8bcecd848
--- /dev/null
+++
b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/completion-item.spec.ts
@@ -0,0 +1,24 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { expectTypeOf, it } from 'vitest';
+
+import { CompletionItem } from './message-paragraph.interface';
+
+it('accepts the Spark and Flink completion payload, which has no meta key', ()
=> {
+ expectTypeOf({ name: 'println', value: 'println'
}).toExtend<CompletionItem>();
+});
+
+it('keeps name and value required', () => {
+
expectTypeOf<CompletionItem>().toHaveProperty('name').toEqualTypeOf<string>();
+
expectTypeOf<CompletionItem>().toHaveProperty('value').toEqualTypeOf<string>();
+});
diff --git
a/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/message-data-type-map.interface.spec.ts
b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/message-data-type-map.interface.spec.ts
index ec7797ecba..f4013f2c8c 100644
---
a/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/message-data-type-map.interface.spec.ts
+++
b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/message-data-type-map.interface.spec.ts
@@ -10,7 +10,7 @@
* limitations under the License.
*/
-import { expect, expectTypeOf, it } from 'vitest';
+import { assertType, expectTypeOf, it } from 'vitest';
import { MessageReceiveDataTypeMap } from './message-data-type-map.interface';
import { OP } from './message-operator.interface';
@@ -23,12 +23,10 @@ it('declares the asymmetric paragraph output payloads sent
by the server', () =>
index: 0,
data: 'chunk'
};
- const update: MessageReceiveDataTypeMap[OP.PARAGRAPH_UPDATE_OUTPUT] = {
+ assertType<MessageReceiveDataTypeMap[OP.PARAGRAPH_UPDATE_OUTPUT]>({
...append,
type: DatasetType.TEXT
- };
- expect(append).not.toHaveProperty('type');
- expect(update.type).toBe(DatasetType.TEXT);
+ });
expectTypeOf<MessageReceiveDataTypeMap[OP.PARAGRAPH_APPEND_OUTPUT]>().toEqualTypeOf<ParagraphAppendOutput>();
expectTypeOf<MessageReceiveDataTypeMap[OP.PARAGRAPH_APPEND_OUTPUT]>().not.toHaveProperty('type');
expectTypeOf<MessageReceiveDataTypeMap[OP.PARAGRAPH_UPDATE_OUTPUT]>().toEqualTypeOf<ParagraphUpdateOutput>();
diff --git
a/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/notebook-wire-fields.spec.ts
b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/notebook-wire-fields.spec.ts
index 471d556498..142f90c087 100644
---
a/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/notebook-wire-fields.spec.ts
+++
b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/notebook-wire-fields.spec.ts
@@ -10,7 +10,7 @@
* limitations under the License.
*/
-import { expect, expectTypeOf, it } from 'vitest';
+import { assertType, expectTypeOf, it } from 'vitest';
import { EditorSettingReceived, ImportNote, Note } from
'./message-notebook.interface';
import { AngularObjectRemove, ImportParagraphItem, ParagraphItem } from
'./message-paragraph.interface';
@@ -45,7 +45,7 @@ it('separates received wire fields from backward-compatible
import input', () =>
lineNumbers: false,
fontSize: 9
} satisfies ImportParagraphItem;
- const importWithoutVersion: ImportNote = {
+ assertType<ImportNote>({
note: {
paragraphs: [legacyImportParagraph],
name: 'Imported note',
@@ -63,20 +63,18 @@ it('separates received wire fields from backward-compatible
import input', () =>
},
info: {}
}
- };
+ });
expectTypeOf<ImportNote['note']>().toHaveProperty('version').toEqualTypeOf<string
| undefined>();
expectTypeOf<ImportNote['note']['paragraphs'][number]>()
.toHaveProperty('progress')
.toEqualTypeOf<number | undefined>();
- expect(importWithoutVersion.note).not.toHaveProperty('version');
-
expect(importWithoutVersion.note.paragraphs[0]).not.toHaveProperty('progress');
});
it('accepts the personalized GET_NOTE response without a version', () => {
// NotebookService.getNote returns Note.getUserNote for personalized
notebooks.
// That copy is constructed with Note(), so its nullable version is omitted
by Message serialization.
- const personalizedNote: Note = {
+ assertType<Note>({
note: {
paragraphs: [],
name: 'Personalized note',
@@ -94,8 +92,7 @@ it('accepts the personalized GET_NOTE response without a
version', () => {
},
info: {}
}
- };
+ });
expectTypeOf<NonNullable<Note['note']>['version']>().toEqualTypeOf<string |
undefined>();
- expect(personalizedNote.note).not.toHaveProperty('version');
});
diff --git a/zeppelin-web-angular/projects/zeppelin-sdk/tsconfig.spec.json
b/zeppelin-web-angular/projects/zeppelin-sdk/tsconfig.spec.json
index 3011c5aeac..436d2bfdf2 100644
--- a/zeppelin-web-angular/projects/zeppelin-sdk/tsconfig.spec.json
+++ b/zeppelin-web-angular/projects/zeppelin-sdk/tsconfig.spec.json
@@ -4,5 +4,5 @@
"noEmit": true,
"types": ["node"]
},
- "include": ["src/**/*.spec.ts", "type-tests/**/*.ts"]
+ "include": ["src/**/*.spec.ts"]
}
diff --git
a/zeppelin-web-angular/projects/zeppelin-sdk/type-tests/completion-item-meta.ts
b/zeppelin-web-angular/projects/zeppelin-sdk/type-tests/completion-item-meta.ts
deleted file mode 100644
index d61cce076e..0000000000
---
a/zeppelin-web-angular/projects/zeppelin-sdk/type-tests/completion-item-meta.ts
+++ /dev/null
@@ -1,24 +0,0 @@
-/*
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- * http://www.apache.org/licenses/LICENSE-2.0
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-import { CompletionItem } from '../src/interfaces/message-paragraph.interface';
-
-// Spark and Flink interpreters build InterpreterCompletion(name, value,
null), and
-// NotebookServer's Gson omits null fields, so their completion payloads have
no `meta` key.
-export const sparkFlinkCompletion: CompletionItem = { name: 'println', value:
'println' };
-
-// `name` and `value` are always supplied by production construction sites and
must stay required.
-// @ts-expect-error `name` is required
-export const missingName: CompletionItem = { value: 'println' };
-
-// @ts-expect-error `value` is required
-export const missingValue: CompletionItem = { name: 'println' };
