Repository navigation
feat(article): frozen Wikipedia fixtures and structural parity - #50
Closed
Willi363363 wants to merge 1 commit into
Closed
Willi363363 wants to merge 1 commit into
Willi363363 wants to merge 1 commit into
Conversation
Steps 3.1 and 3.3 of phase 3. This phase's first pitfall says the parity test is the piece to write first; this is that piece. C3.2 is the invariant that cost the worst bug in the project's history — the positions were drawn at random and the player was graded on paragraphs the model never touched. So the collection does not return two lists that have to stay aligned. It returns one object holding the loaded document and its nodes, with the text derived from those same nodes: there is no second list to drift, and no second tree. The fixtures are real. Two rendered pages from fr.wikipedia.org, reduced to their first paragraphs so they stay readable in a diff, every paragraph byte-for-byte as MediaWiki served it, revision recorded, licence stated — they carry the shapes a hand-written fixture does not think to contain: references, non-breaking spaces, nested inline tags, image captions, and paragraphs from 38 to 2003 characters. The duplicated-variant case is constructed, because `action=parse` does not serve the mobile/desktop duplication, and the file says so. The parity test was checked against three regressions, and catches all three: a re-parse between collection and injection — the pitfall named in the sheet — a filter applied after collection, and the deduplication removed. Two lint rules earned their place here. `no-irregular-whitespace` caught non-breaking spaces written literally in a character class: an invisible character in a character class is a character class nobody can review, so the codepoints are escapes now. And Prettier reformatted the fixtures, which would have made "byte-for-byte" false and silently changed the text the parity test reads — the directory is excluded, and the files were regenerated from source. `domhandler` is declared rather than reached through cheerio's inference. My first attempt avoided the dependency and produced a signature nobody could read; the dependency is the honest answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01533WTTbgmfCaDP5tmK8Gkv
PR Summary by QodoArticle: add frozen Wikipedia fixtures and enforce paragraph/node parity
AI Description
Diagram
High-Level Assessment
Files changed (15)
|
Code Review by Qodo
1. Brittle fixture path join
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+19
to
+22
| const FIXTURES = fileURLToPath(new URL('../fixtures/', import.meta.url)); | ||
|
|
||
| function fixture(name: string): string { | ||
| return readFileSync(`${FIXTURES}${name}`, 'utf8'); |
There was a problem hiding this comment.
1. Brittle fixture path join 🐞 Bug ☼ Reliability
fixture(name) builds file paths via string concatenation, which implicitly relies on FIXTURES always ending with a path separator; if that invariant changes (or on edge platform/path normalization cases), reads will fail with ENOENT and the suite becomes fragile.
Agent Prompt
### Issue description
`fixture(name)` currently uses string concatenation to build a path: ``readFileSync(`${FIXTURES}${name}`)``. This is brittle because it depends on `FIXTURES` having a trailing separator.
### Issue Context
`FIXTURES` is derived from `fileURLToPath(new URL('../fixtures/', import.meta.url))`. The test suite should not depend on subtle trailing-slash behavior.
### Fix Focus Areas
- packages/article/src/paragraphs.test.ts[19-23]
### Suggested fix
Prefer a join that is correct regardless of trailing slashes:
- Option A (recommended): `import { join } from 'node:path'` and use `readFileSync(join(FIXTURES, name), 'utf8')`.
- Option B: keep everything URL-based for `fixture()` by using `readFileSync(new URL(`../fixtures/${name}`, import.meta.url), 'utf8')` and keep `FIXTURES` only for `readdirSync`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+25
to
+32
| const REAL_PAGES = readdirSync(FIXTURES) | ||
| .filter((name) => name.endsWith('.html') && name !== 'variants.html') | ||
| .sort(); | ||
|
|
||
| describe('the fixtures are real pages', () => { | ||
| it('found them', () => { | ||
| expect(REAL_PAGES).toEqual(['chat.html', 'chocolat.html']); | ||
| }); |
There was a problem hiding this comment.
2. Hard-coded fixture inventory 🐞 Bug ⚙ Maintainability
The test suite asserts the fixture directory contains exactly chat.html and chocolat.html, so adding any additional real-page fixture later will fail unrelated tests and slow iteration on fixtures.
Agent Prompt
### Issue description
The test `expect(REAL_PAGES).toEqual(['chat.html', 'chocolat.html'])` will fail if any new real `.html` fixture is added (even if it’s valid and needed for future regressions).
### Issue Context
`REAL_PAGES` is derived from the fixtures directory listing. This makes the test suite sensitive to fixture set growth.
### Fix Focus Areas
- packages/article/src/paragraphs.test.ts[25-32]
### Suggested fix
Change the assertion to require presence rather than exact equality, e.g.:
- `expect(REAL_PAGES).toEqual(expect.arrayContaining(['chat.html','chocolat.html']))`
- and optionally keep a separate assertion for “at least N real pages” if you want a guardrail.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
|
Closed by the stack collapse, not abandoned. Every commit of this pull request is in Three commit messages were reworded on the way — a This description and its review thread stay readable here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Steps 3.1 and 3.3 of
plans/rewrite/phase-03-article.md. Phase 3 opens here.feat/rewrite-phase-3branches off the top of phase 2 (#48). This is a stepbranch onto it. Note: like the earlier phase branches, it has no PR of its
own yet — #49 is the integration PR that carries phases 1 and 2 into
staging,and phase 3 will need its own when it closes.
The parity test, first
This phase's opening pitfall: "The parity test first. It is the piece to write
first; the whole rest of the chain rests on it."
C3.2 is the invariant that cost the worst bug in the project's history — the
positions were drawn at random and the player was graded on paragraphs the model
never touched.
So collection does not return two lists that have to stay aligned. It returns
one object holding the loaded document and its nodes, and the text is derived
from those same nodes:
There is no second list to drift and no second tree. Injection writes to
nodes[i]and re-collecting the emitted HTML finds the change at indexi—which is the whole chain, end to end.
Verified against three regressions
writing to index i changes paragraph i and no other(both fixtures)every text belongs to the node at the same index(both fixtures)drops variants served twiceThe fixtures are real
Two rendered pages from
fr.wikipedia.org, reduced to their first paragraphs sothey stay readable in a diff. Every paragraph is byte-for-byte as MediaWiki
served it, with its revision number recorded and its licence stated — C6.1
applies to a fixture like any other reuse.
They carry what a hand-written fixture does not think to contain:
Both include a paragraph over 1000 characters, which step 3.4 needs for the
truncation bug. The short ones (38, 51) sit either side of the 50-character floor.
variants.htmlis constructed and says so:action=parsedoes not serve themobile/desktop duplication, so that shape is built around real paragraphs taken
from
chat.htmlrather than around"aaa".Two lint rules earned their keep
no-irregular-whitespacecaught non-breaking spaces written literally insidea character class. An invisible character in a character class is a character
class nobody can review — the codepoints are escapes now
(
U+00A0,U+202F,U+2009,U+2007), with a comment naming each.Prettier reformatted the fixtures. That would have made "byte-for-byte" false
and silently changed the text the parity test reads, since HTML whitespace is
exactly what this test is about. The directory is excluded — same reason as
drizzle's snapshots — and the files were regenerated from source.
A dependency I first tried to avoid
domhandlerholds cheerio's node types. My first attempt reached for themthrough inference and produced a signature nobody could read, plus
TS2742: the inferred type cannot be named without a reference to domhandler.Declaring it is the honest answer: this module handles nodes.
Checks
Two new dependencies:
cheerio(named by the phase objective) anddomhandler(its node types).
Not run: the Python backend tests — no
pytesthere. This touches no Python.Note: the commit hook refused my first subject at 74 characters. Shortened, not
bypassed.
Next
3.2 (MediaWiki client with explicit language and user-agent), 3.4 (falsification
via
generateObject), 3.5 (end-to-end parity), 3.6 (Redis cache), 3.7 (counters).🤖 Generated with Claude Code
https://claude.ai/code/session_01533WTTbgmfCaDP5tmK8Gkv