Normalize HTML when comparing featured item content (unblocks test-functional on all PRs) - #485
Open
ErykKul wants to merge 1 commit into
Open
Normalize HTML when comparing featured item content (unblocks test-functional on all PRs)#485ErykKul wants to merge 1 commit into
ErykKul wants to merge 1 commit into
Conversation
…er-formatted string
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is test-only, narrowly scoped to the failing assertion, and includes targeted unit coverage to prevent the normalizer from masking real content regressions.
Pull request overview
This PR stabilizes the SDK’s functional test suite by replacing a brittle exact-string assertion (server-sanitized HTML) with a normalization-based comparison that tolerates harmless formatting drift introduced by upstream Dataverse/HTML sanitizer changes.
Changes:
- Add a
normalizeHtmltest helper to canonicalize HTML for comparisons (attribute ordering, block-boundary whitespace, tag/attr case; preserves whitespace in<pre>/<textarea>and between inline elements). - Add unit tests covering both “allowed drift” and “must-detect” differences for the normalizer.
- Update
UpdateCollectionFeaturedItemsfunctional test to compare normalized HTML instead of raw strings.
File summaries
| File | Description |
|---|---|
test/unit/testHelpers/htmlNormalizer.test.ts |
Adds unit coverage to ensure the HTML normalizer absorbs only intended formatting differences and still detects meaningful changes. |
test/testHelpers/html/htmlNormalizer.ts |
Introduces the normalizeHtml helper used to make HTML assertions robust to server-side serialization differences. |
test/functional/collections/UpdateCollectionFeaturedItems.test.ts |
Switches the featured-item content assertion to use normalizeHtml(...) on both actual and expected HTML. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
What this PR does / why we need it:
test-functionalis currently red on every open PR in this repo. A single test fails —UpdateCollectionFeaturedItems › should successfully update the featured items of a collection— and it fails identically on unrelated branches (for example #484, a dependabotjs-yamlbump, and #403). The last fully green PR run was 2026-09-03; everything after it fails on this one test.Nothing about the SDK changed. The assertion compares the server's sanitized HTML against
EXPECTED_CONTENT_FIELD_WITH_ALL_TAGS, a hard-coded snapshot of exactly how an older Dataverse serialized that markup. Thegdcc/dataverse:unstableimage the test containers boot now serializes the same content differently in two ways: attributes come back in alphabetical order (class,href,rel,targetinstead oftarget,rel,class,href), and list items are pretty-printed across separate lines instead of on one. Both are hallmarks of a jsoup upgrade upstream. The content is unchanged — only its formatting is — but an exact string comparison cannot tell the difference.This PR replaces the exact-string comparison with a normalizing one, so the test asserts on the HTML the server returns rather than on the particular way that server chose to print it.
Which issue(s) this PR closes:
None filed — this surfaced as CI breakage across all open PRs.
Special notes for your reviewer:
A note on the history, since it looks odd in the log: this change was pushed straight to
developby mistake (commite204481b) and reverted immediately afterwards (9e49b8b6), so both appear in develop's history. The branch was created trackingorigin/developinstead of a branch of its own name, and a push followed that upstream. The commit in this PR is the same change re-applied on top of the revert — its content is byte-identical to the commit that was tested, and it is going through review here as it should have in the first place.normalizeHtml(test/testHelpers/html/htmlNormalizer.ts) sorts attributes, drops whitespace at block-element boundaries, and lowercases tag and attribute names. It deliberately preserves whitespace inside<pre>and<textarea>, and whitespace that separates inline elements, because both are significant — the fixture's code block depends on the former.The risk with any normalizer is that it quietly turns the assertion into a tautology. The 12 unit tests are split to guard against exactly that: six cover drift the helper should absorb (attribute order, indentation between and inside block elements, tag case, and the two real fixture constants), and six cover differences it must still catch — changed text, changed attribute value, a dropped attribute, changed structure, a changed tag, collapsed
<pre>whitespace, and a lost space between inline elements. Those tests were written before the implementation existed and confirmed failing first.Only the one brittle assertion is touched. The other featured-item tests (
GetCollectionFeaturedItems,DeleteCollectionFeaturedItem,CollectionsRepository) compare a single flat<p class="rte-paragraph">Test content</p>with one attribute and no nesting, so neither attribute reordering nor pretty-printing can affect them; they pass in CI and are left alone.Two things worth knowing beyond this change. First, this fixes the symptom, not the cause:
unstablewill keep drifting, and pinningDATAVERSE_IMAGE_TAGintest/environment/.envis the broader fix if it keeps costing us. Second, I hit a second instance of the same class of problem locally —MetadataBlocksRepositoryassertsgetAllMetadataBlocks().lengthis 7, butDatasetsRepository.test.tsloads extra metadata blocks and never removes them, so the count is 9 whenever Jest happens to schedule the datasets suite first. That one is order-dependent and out of scope here, but it will bite intermittently.Suggestions on how to test this:
Is there a release notes or changelog update needed for this change?:
No — test-only change, no effect on the published package.
Additional documentation:
None.
AI-assistance disclosure
Some parts of this work were developed with the help of Claude (Anthropic) via Claude Code.
Reviewer attention is still required: AI-assisted code is still author-owned, and we've reviewed every diff that landed. Flagging this so reviewers can apply whatever scrutiny they reserve for AI-touched changes.