Skip to content

Six unit specs still hand-roll the comment stripper the helpers module exists to be #2678

Description

@AndriiPasternak31

What

src/frontend/tests/unit/helpers/stripComments.js was extracted in #2161, and its own docstring
says why: "Extracted here because it was about to gain a third copy, and the copies carried a
real defect."
25 specs now import it. Six still define their own:

Spec Shape
agentDetailDeepLink.spec.js local function stripComments
gridStorageKeys.spec.js local function stripComments
mountListenerOrdering.spec.js local function stripComments
retiredEntitlementGates.spec.js local function stripComments
viewModeStructure.spec.js local function stripComments
portalVoiceMode.spec.js local stripHtmlComments regex-loop + stripComments
portalComposerAlignment.spec.js local stripHtmlComments, used only by composerForm

Why it matters

The copies are not equivalent. portalVoiceMode.spec.js uses the loop-the-regex form, and the
shared helper's docstring explicitly records that looping does not fix the residue it was
written for: <!<!---->--> reduces to <!-->, which still contains <!-- and no longer
matches, so the loop stops with an opener intact. CodeQL flags that shape as
js/incomplete-multi-character-sanitization — it did so on #2662's new spec, which then
re-derived the scan-based fix by hand rather than importing the module that already had it.

Nothing is rendered from these strings, so this is not an injection risk in the product. The
cost is a source-structure guard that can fail — or pass — on leftover prose, which is the exact
failure the stripping exists to prevent.

Done when

  • All seven specs import ./helpers/stripComments; no local copy remains under tests/unit/.
  • Each is verified equivalent by the suite passing unchanged, not by inspection.
  • A guard (an AST or grep check in the existing test-placement guard family) fails the build on
    a new local stripComments / stripHtmlComments under tests/unit/, so the eighth copy is
    refused rather than reviewed.

Context

Found during the #2662 review (PR #2665). #2662 fixed its own copy — the new
baseSelectChevron.spec.js — by importing the helper; the six pre-existing ones were left
deliberately, as they predate that branch and are not what a composer fix should touch.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    complexity-lowComplexity: low (board points 1-3)priority-p3Nice-to-havestatus-readyGreenlit and ready for development (vetted; counterpart to status-incubating)theme-devexTheme: DevExtype-refactorCode improvement

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions