Skip to content

Normalize HTML when comparing featured item content (unblocks test-functional on all PRs) - #485

Open
ErykKul wants to merge 1 commit into
developfrom
fix/normalize-featured-item-html-comparison
Open

Normalize HTML when comparing featured item content (unblocks test-functional on all PRs)#485
ErykKul wants to merge 1 commit into
developfrom
fix/normalize-featured-item-html-comparison

Conversation

@ErykKul

@ErykKul ErykKul commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

test-functional is 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 dependabot js-yaml bump, 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. The gdcc/dataverse:unstable image the test containers boot now serializes the same content differently in two ways: attributes come back in alphabetical order (class,href,rel,target instead of target,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 develop by mistake (commit e204481b) and reverted immediately afterwards (9e49b8b6), so both appear in develop's history. The branch was created tracking origin/develop instead 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: unstable will keep drifting, and pinning DATAVERSE_IMAGE_TAG in test/environment/.env is the broader fix if it keeps costing us. Second, I hit a second instance of the same class of problem locally — MetadataBlocksRepository asserts getAllMetadataBlocks().length is 7, but DatasetsRepository.test.ts loads 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:

npm install
npm run test:unit        # includes the 12 new normalizer tests
npm run test:functional  # the previously failing suite

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.

@ChengShi-1
ChengShi-1 requested a lite review from Copilot September 9, 2026 18:11
@ChengShi-1 ChengShi-1 added Size: 3 A percentage of a sprint. 2.1 hours. FY27 Sprint 6 FY27 Sprint 6 (2026-09-09 - 2026-09-23) labels Sep 9, 2026
@ChengShi-1 ChengShi-1 added the SPA label Sep 9, 2026
@ChengShi-1 ChengShi-1 moved this to In Review 🔎 in IQSS Dataverse Project Sep 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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 normalizeHtml test 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 UpdateCollectionFeaturedItems functional 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FY27 Sprint 6 FY27 Sprint 6 (2026-09-09 - 2026-09-23) Size: 3 A percentage of a sprint. 2.1 hours. SPA

Projects

Status: In Review 🔎

Development

Successfully merging this pull request may close these issues.

3 participants