You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "voidmatcha (via GitHub)" <gi...@apache.org> on 2026/09/04 14:46:00 UTC
[PR] [ZEPPELIN-6660] Add notebook parity registry [zeppelin]
voidmatcha opened a new pull request, #5456:
URL: https://github.com/apache/zeppelin/pull/5456
### What is this PR for?
This PR records the current Angular Notebook behaviour in a reusable parity registry.
The editable source is JSON and the human-readable Markdown table is generated from it. Each scenario has a stable `NB-PARITY-###` identifier and links either to an existing Playwright E2E test or to the Jira issue that tracks the remaining work.
The validator rejects drift between JSON and generated Markdown, duplicate or invalid scenario identifiers, unsupported coverage states, a `covered` claim without an executable Playwright test carrying the same identifier, incomplete scenarios without a Jira issue, and invalid evidence or test paths.
Existing Notebook Playwright test titles now carry the stable scenario identifiers, and the frontend CI workflow runs the registry check.
This does not implement missing E2E behaviour or claim that React Notebook parity is complete. It establishes the characterization baseline that later Angular, Shared Core, and React work can compare against.
The generated registry and validation approach follows Apache Ambari's React parity matrix work:
* [React parity matrix](https://github.com/apache/ambari/blob/trunk/docs/frontend-refactor/react-current/README.md)
* [Parity matrix validator](https://github.com/apache/ambari/blob/trunk/docs/frontend-refactor/react-current/tools/validate-react-parity-matrix.mjs)
* [Related implementation PR #4176](https://github.com/apache/ambari/pull/4176)
### What type of PR is it?
Improvement
### Todos
* [x] Add a JSON Notebook parity registry
* [x] Generate the reviewable Markdown view from the registry
* [x] Add generation and validation tests, including failure cases
* [x] Link existing Notebook Playwright coverage through stable scenario identifiers
* [x] Run the registry validation in frontend CI
### What is the Jira issue?
ZEPPELIN-6660
### How should this be tested?
```bash
cd zeppelin-web-angular
npm run check:notebook-parity-scenarios
```
The command passes after rebasing on the latest `origin/master`: 15 validator tests pass and the checked-in registry matches its generated Markdown view.
The `quick` GitHub Actions workflow has passed. The `frontend` and `core` workflows are still running for the rebased commit.
### 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. `notebook-parity.md` is the generated review document for the registry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6660] Add notebook parity registry [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5456:
URL: https://github.com/apache/zeppelin/pull/5456#issuecomment-5552971825
A few suggestions.
**1. Move the schema definition to a declarative form**
`validateRegistry` is effectively the schema, and most of it (field types, enums, patterns, the per-`status` requirements) can be declared instead. Moving that to a JSON Schema file and validating with Ajv would leave only the rules that have to look at the outside world: path existence, git commit reachability, and the `test()` declaration check.
Adding `$schema` to the data file also surfaces errors in the editor while a contributor is editing, rather than only in CI. The `status` branching maps to `if/then`, or to `discriminatedUnion` if you prefer zod. With a schema file, `schemaVersion` becomes redundant with `$schema`.
**2. Playwright tags instead of test titles**
Putting the identifier in the title and matching it as a substring means routine title cleanups break the registry. Playwright here is 1.55.1, so tags are available:
```ts
test('should display notebook container with proper structure', { tag: '@NB-PARITY-001' }, async () => {});
```
**3. Does the role axis need to be required on every scenario?**
NB-PARITY-001 and 060 are `allow` for every role, so there is no permission dimension there. And 003 (entering edit mode) marks reader as `deny` while 010 and 011, which happen inside the same editor, mark it `n/a`, so "not relevant" and "should be denied" are mixed together. Would making the field optional, or requiring it only for `area: "permission"` scenarios, work better?
`roleVerification` also allows only `unverified` and `not-applicable`, so there is no value to record a permission test once one is written.
**4. Duplicated strings that are not identifiers**
Referencing scenarios by `id` is the right call, but the same principle is not applied one level down, so several values are copied verbatim.
- `uncoveredOutcomes` repeats whole sentences from `observableOutcomes`. Could the outcomes carry ids and be referenced instead?
- `evidence` points at tests on `covered` rows and at production sources on `gap` rows. 060 is a `gap` yet lists an existing spec as evidence, which makes it hard to tell whether that test covers the scenario. Splitting "where it is implemented" from "what demonstrates it" would let each side carry its own rule.
- `projects` is a hand copy of `test.skip(browserName !== 'chromium', …)` in the spec, and it is not checked. Removing that skip leaves the registry stale and CI green.
**5. Consistency with `e2e/AGENTS.md`**
`e2e/AGENTS.md:124` currently says "no cross-framework parity project in this config… do not build parity infrastructure ahead of need." If this PR is the decision to change that stance, could that paragraph be updated in the same PR? The convention that test titles must keep the identifier is not documented anywhere either, so a contributor following that file would not know why CI broke.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6660] Add notebook parity registry [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee merged PR #5456:
URL: https://github.com/apache/zeppelin/pull/5456
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6660] Add notebook parity registry [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5456:
URL: https://github.com/apache/zeppelin/pull/5456#issuecomment-5556331146
@tbonelee Thanks for the review. I addressed the suggestions:
* Moved the registry structure to JSON Schema with Ajv
* Replaced IDs in test titles with Playwright tags
* Made role metadata optional and added the `verified` state
* Added stable outcome IDs, split implementation and verification evidence, and removed duplicated browser project metadata
* Updated `e2e/AGENTS.md`
I also hardened the validator and added regression coverage. All 21 notebook parity checks pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6660] Add notebook parity registry [zeppelin]
Posted by "tbonelee (via GitHub)" <gi...@apache.org>.
tbonelee commented on PR #5456:
URL: https://github.com/apache/zeppelin/pull/5456#issuecomment-5604788714
Merged into master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org