Skip to content

SettlementTable.test.tsx passes Element where HTMLElement is required at 5 sites — queries are returning a weaker type than the assertions need #430

Description

@Jagadeeshftw

Priority: Medium  ·  Area: Test correctness  ·  Est. effort: 5–8 h

📌 Problem

src/components/SettlementTable.test.tsx produces 5 TS2345 errors — at lines 72, 269, 270, 271 and 272:

Argument of type 'Element' is not assignable to parameter of type 'HTMLElement'.

Something in the test is producing a plain Element — typically container.querySelector(...), .closest(...), .parentElement, or iterating children — and passing it into an API that requires HTMLElement, such as a @testing-library/jest-dom matcher or a within(...) scope.

The tests pass, because CI never type-checks test files (tracked separately). But the type error is a real signal, and the fix is not a cast:

  • querySelector returning Element | null means the query result may be null. Casting to HTMLElement silences both the type mismatch and the null check, so a query that stops matching after a markup change fails with a confusing runtime error instead of a clear assertion failure.
  • Reaching for querySelector at all usually means the test is asserting on DOM structure rather than on what a user perceives. Testing Library's role/label queries return HTMLElement directly and are resilient to markup changes.

The clustering — four consecutive lines at 269–272 — suggests one block doing the same thing four times, so a single change likely fixes four of the five.

🎯 Design decision required

State and defend:

  1. Query strategy. Should these become getByRole / getByLabelText / within(...) queries, or is direct DOM traversal genuinely necessary here? Prefer semantic queries — and note this table is also covered by the accessibility issue in this repo, so if a role query is not available, that may itself be an a11y finding worth reporting.
  2. Null handling. Whatever you use, a query that fails to match must produce a clear failure. Say how yours does.
  3. The 269–272 cluster. Establish whether one root cause explains all four and fix it once rather than four times.

🧩 Requirements and context

  • Do not fix this with as HTMLElement or a non-null assertion. That hides the null case and defeats the purpose.
  • Assertions must keep testing the same behaviour. If a semantic query changes what is asserted, explain why the new assertion is equivalent or better.
  • If a role- or label-based query is unavailable because the markup lacks the necessary semantics, report that as an accessibility gap and cross-reference the a11y issue.
  • All existing SettlementTable assertions must still pass.
  • Do not change the component to make the tests easier, unless you are fixing a genuine a11y gap — in which case say so.

🛠️ Suggested execution

  1. Run npx tsc --noEmit and confirm the five errors.
  2. Read each site and identify what it is really asserting.
  3. Determine whether one root cause covers 269–272.
  4. Replace with semantic queries where possible.
  5. Report any markup that could not be queried semantically.

✅ Acceptance criteria

  • npx tsc --noEmit reports zero errors for SettlementTable.test.tsx; the repo-wide count drops by 5.
  • No as HTMLElement cast or non-null assertion was used.
  • The PR states what each of the five sites was asserting and how that is preserved.
  • Whether one root cause explains the 269–272 cluster is established and stated.
  • Any markup lacking semantics for a role/label query is reported as an a11y gap.
  • All existing assertions still pass; the full suite is green.

🚫 Out of scope

  • MetricsBar.test.tsx's errors — separate issue.
  • Adding the typecheck CI gate — separate issue.
  • Adding accessibility tooling — separate issue, though report findings to it.

🧪 Verification

npm ci
npx tsc --noEmit 2>&1 | grep -c "error TS"          # report before and after
npx tsc --noEmit 2>&1 | grep SettlementTable         # expect empty
npm test src/components/SettlementTable.test.tsx
npm test

📤 What your PR must include

  1. The five errors before, and the repo-wide count before/after.
  2. What each site was asserting and how it is preserved.
  3. Whether one root cause covered the 269–272 cluster.
  4. Any accessibility gap discovered.
  5. Closes #<n>.

🔒 Security notes

Tests that assert on DOM structure rather than user-perceivable output drift out of alignment with what users actually experience: markup can be refactored while the tests still pass, or the tests break while nothing user-visible changed. For a settlement table, the assertions worth having are that a user can find a settlement's status and amount — which is the same property the accessibility work in this repo is trying to establish.

📋 Guidelines

  • Minimum 95% test coverage on changed lines
  • Clear documentation
  • Timeframe: 96 hours from assignment
  • One logical change per commit; no merge commits

💬 Join our community

Working on this, or want to sanity-check your approach before you start? Come and ask — the maintainers are there and happy to help.

Telegram: https://t.me/Grainlify

Activity

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

Metadata

Metadata

Assignees

Labels

GrantFox OSSGrantFox open-source programMaybe RewardedGrantFox: potentially rewarded contributionThird CampaignGrantFox third campaign issuebugSomething isn't workingpriority:mediumMedium difficulty / self-contained but non-trivial

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions