aglinxinyuan commented on PR #8133:
URL: https://github.com/apache/texera/pull/8133#issuecomment-5479746349
Thanks for the review — three applied, two refused, and one of the
applications turned out to cost something I had to put back. Pushed as
`9154c653` and `3a7d25f4`.
**Applied.**
- **`share-access:740`** (DOM-order-dependent `[privateButton,
publicButton]` destructure) — added an `accessButton("Private" | "Public")`
helper that filters `button.access-button` by its visible `.button-text-header`
text and asserts the match is unique. **No `data-testid`** — that label already
exists in the shipped template.
- **`share-access:75`/`:100`** (shared mutable `fixture`) — applied in the
narrow sense: the shared `fixture` is now cleared in the existing `beforeEach`,
alongside the `TestBed.resetTestingModule()` already there, with a note on the
declaration.
- **`user-workflow-list-item:507`** (tooltip-text selection) — added
`onlyByTooltip(pred)`, which wraps the pre-existing `byTooltip()` and asserts
exactly one match, and used it at the three sites this PR introduces.
**The label refactor dropped a mutation kill, and that's worth flagging.**
Replacing the positional destructure with a label lookup removed the
DOM-**order** constraint that the destructure had been carrying implicitly — a
mutant swapping the two buttons in the template survived. Rather than revert
the selector, I added a test that reads the two `.button-text-header` labels in
document order and asserts `["Private", "Public"]`, so order and identity now
fail independently. Net effect: the fragility you flagged is gone *and* the
ordering contract is explicit instead of accidental.
**Refused: moving `vi.useFakeTimers()` after `await createFixture()`.**
`frontend/vitest.config.ts` sets no `fakeTimers` block, so the default `toFake`
list applies — `setTimeout`/`setInterval`/`setImmediate`/`Date`, but **not**
`queueMicrotask` or `process.nextTick`. The only `await` inside `createFixture`
is `TestBed.configureTestingModule(...).compileComponents()`, which resolves on
the microtask queue. So installing the clock first can't interfere with fixture
setup, and it *does* keep any timer the component schedules during creation on
the fake clock — which is the more hermetic order, not the less.
**Refused: `setupComponent` returning `{ fixture, component }`.** The stated
benefit is future parallelisation, and it isn't available here: the Angular
unit-test builder forks one Vitest worker per spec **file**, and specs within a
file run sequentially in a single module registry. Returning the fixture would
change nothing about concurrency, leaving a style preference that touches every
call site in a 780-line spec.
**Refused: `data-testid` / accessible-label hooks.** Both are production
template edits, and this PR is test-only with an empty production diff — which
is the basis it was approved on. Where a stable non-production hook already
existed (`share-access`'s label text) the underlying concern *was* addressed.
For the history action the only alternative is `nzType="history"`, which is a
template attribute exactly like the tooltip and pins the glyph rather than the
text a user actually reads — not more stable, just different.
5 spec files pass, `yarn format:ci` clean, production diff empty and
independently re-checked at the merge base.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
