aglinxinyuan opened a new pull request, #7843:
URL: https://github.com/apache/texera/pull/7843
### What changes were proposed in this PR?
17 tests for `code-editor.component.ts`, covering the four things PR #7735
declared unreachable.
**#7735's count was right and its explanation was wrong.** It named the
`monacoWorkerFactory` label switch, the dynamic codingame extension imports,
the retry-clearing `catch` and the LSP timeout reject — exactly 10
union-uncovered lines, and exactly the right ten (279, 281, 283, 285, 287,
302-303, 310-311, 377). But its stated reason, *"All sit behind
`ensureVscodeApiStarted()`, a process-wide singleton both suites stub; reaching
them means booting the real codingame stack"*, is false on both halves:
- The jsdom spec stubs **nothing**. A search for `vi.mock` / `vi.doMock`
over it returns zero hits, and there is no `ensureVscodeApiStarted` stub — the
only mentions are the new tests calling it.
- `ensureVscodeApiStarted()` genuinely resolves under jsdom; a real monaco
editor mounts over a real text model.
Nor is `getEnhancedMonacoEnvironment()` a barrier: it assigns
`globalThis.MonacoEnvironment = {}` when that global is undefined and returns
that same global, and `configureMonacoWorkers()` runs at the top of `start()`
before any dynamic import, so the factory installs
`MonacoEnvironment.getWorker` on the way in regardless of what fails later.
Three of the four items are now covered by plain jsdom tests. **If that
paragraph of #7735 is ever quoted, treat it as retracted.**
### The number Codecov will show is inflated, so here are both
| Measurement | Before | After |
|---|---|---|
| Codecov-visible (jsdom lcov — the only one `build.yml` uploads) | 192/229
= 83.8% | **217/217 = 100%** |
| True union (jsdom + browser) | 237/247 = 95.9% | **245/247 = 99.2%** |
**The honest gain is 8 lines.** 29 of the 37 lines this bundle newly covers
in jsdom were already exercised by browser-mode tests whose lcov is never
uploaded — `build.yml` runs the browser target with no `--coverage`. Branches
80.7% → 98.2% and functions 76.7% → 100% on the jsdom target.
The durable value is less the 8 lines than the 17 tests now pinning
behaviour that had no assertions at all: diff-editor side assignment, disposal
ordering, the re-tokenize loop bound, and the retry-memo clearing.
Tests 51 → 68 `it` blocks (65 → 82 cases; the new blocks are partly
table-driven).
### Verification
18 mutations, **17 killed, 1 unkillable survivor**, each applied one at a
time from a scratch-dir snapshot with a uniqueness assertion on every anchor,
and the production md5 verified against the HEAD copy after every revert.
| Mutation | Killed by |
|---|---|
| **exchange** the `editorWorkerService` and `TextMateWorker` case bodies |
points each monaco worker label at its own worker entry point |
| `default:` returns the editor worker instead of throwing | refuses a
worker label it has no entry point for |
| delete the cached-start-promise reset from the catch | drops the cached
start promise when the vscode API fails so a later open retries |
| **exchange** the diff editor's modified and original sides | diffs the
version being viewed against the latest editing version |
| **exchange** the two interpolations in the editor model uri | binds the
shared yjs text to the mounted editor and hands that editor to the debugger |
| negate the `readOnly` flag | opens the editor read-only when disabled, and
writable when enabled — both directions |
| off-by-one on the re-tokenize loop bound | re-tokenizes every line of the
model once the editor is mounted |
| delete the no-shared-code bail-out in the bring-up subscribe | mounts the
editor but binds nothing when the operator holds no shared code |
| **exchange** detach-after-reattach instead of before | destroys the
previous binding and detaches the previous listener on a second bring-up |
| delete the monaco binding destroy | same test, distinct assertion |
| rethrow instead of swallowing the language-server race failure | keeps the
editor usable when the python language server never answers |
| delete the destroy-time yjs listener detach | detaches the yjs code
listener it attached |
| **exchange** the editor-app and language-client disposal order | shuts the
language client down before the editor |
| drop both no-op rejection handlers from the disposals | same test, after
strengthening — see below |
| retarget the host listener at `window:scroll` | clamps the dialog through
the window resize host listener |
| no-model offset fallback 0 → 1 | inserts at the start of the document when
there is no model to measure against |
| **exchange** the debugger init above the bail-out | proves the three
undefined-assertions are not just reading field defaults |
One mutation failed only to **compile**, which proves nothing; it was
discarded and replaced with a semantic version.
**The survivor, stated plainly:** dropping the module-type option from the
worker construction survives and is unkillable from a spec. The bundler's
worker plugin rewrites the whole expression and supplies the options itself —
the recorded options carry the module type whether or not the source literal is
present. The label to entry-point mapping *is* pinned.
### Two claims inherited from #7735's tests were wrong, and the tests changed
- The worker-wiring test asserted the module-type option, with a comment
claiming every worker has to be a module worker. That is unassertable per the
survivor above, so the comment was replaced with the real explanation rather
than left as a lie in the file.
- The teardown test was named "…and swallows both failures", but deleting
both no-op rejection handlers **survived** — this runner does not fail on those
unhandled rejections. The plain rejected-promise stand-ins were replaced with a
thenable that records whether the caller attached a rejection handler, which
makes that mutation die on an assertion.
### Deliberately not included
Lines 302-303, the codingame python and java default-extension imports,
remain the only uncovered lines in the union. They look like a v8 attribution
artifact rather than dead code: the enclosing `Promise.all([` and the following
`whenReady` sweep are both covered in all three runs, so the imports
demonstrably resolved. Chasing a v8 range quirk is not a coverage win.
Uploading browser-target coverage belongs to #7586, not here. The
coverage-provider `optimizeDeps` entry was applied only to take the union
measurement above, then reverted from a pristine copy; the diff against
`frontend/vitest.browser.config.ts` is empty.
No production file is touched.
### Any related issues, documentation, discussions?
Closes #7842
### How was this PR tested?
```
npx ng test --watch=false --include="**/code-editor.component.spec.ts"
```
```
Test Files 1 passed (1)
Tests 82 passed (82)
```
A full-project run is **required**, not optional, for anything touching a
bundler-rewritten `new Worker(new URL(...))`: `--include` runs one file, but
the suite-wide run changes how the component is chunked, which changes
`import.meta.url`, which changes every rewritten worker URL. One of these tests
passed alone and failed in the full run for exactly that reason — the component
resolved from a hoisted chunk at the server root while the spec's reference
resolved under its own directory, same content hash, different prefix. Fixed by
comparing only the content-hashed chunk name plus query, then re-verified that
the label-mapping mutation still dies.
```
npx ng test --watch=false
```
```
Test Files 201 passed (201)
Tests 4832 passed | 1 skipped (4833)
```
The `code-editor-dialog` directory is green at 130 tests across 4 spec
files, and no later spec shows change-detection fallout. `yarn format:ci`
passes with no files listed.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
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]
