Skip to content

Lint the full JS package in CI, not just src - #984

Merged
aram356 merged 7 commits into
mainfrom
ci-lint-js-tests
Aug 1, 2026
Merged

Lint the full JS package in CI, not just src#984
aram356 merged 7 commits into
mainfrom
ci-lint-js-tests

Conversation

@aram356

@aram356 aram356 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Fixes #985.

Problem

The CI lint gate (npm run lint in format.yml) only covered src/**/*.{ts,tsx}, so eslint errors in test files, the Node build scripts, and config files were invisible: 280 of them had accumulated (npx eslint . on main), and new ones land unnoticed.

Fix

Widen the lint script to eslint . --max-warnings=0 and make the widened surface clean, with no rule exceptions:

  • Tests: all 249 no-explicit-any violations are fixed with real types rather than suppressed. Window mocks are typed as Omit<Window, 'tsjs'> intersections with Partial<TsjsApi> from src/core/types; the prebid hoisted mock gets structural TestPbjs/TestAdUnit/TestBid types; many casts were simply unnecessary and are deleted (untyped vi.fn() already assigns to typeof fetch). One documented as unknown as double-cast remains, for a test that deliberately feeds malformed input. Side effect: repo-wide tsc --noEmit errors drop from 50 to 12, all remaining ones pre-existing in files this PR does not touch.
  • Node scripts: declare ESM-safe Node globals (globals.nodeBuiltin) for *.mjs build scripts and Node-run test harnesses, so process/console/Buffer resolve but CommonJS-only names (__dirname, require) still fail no-undef.
  • Unused args: honor the ^_ underscore convention in @typescript-eslint/no-unused-vars via a top-level rules block covering every linted file, not just *.ts(x).
  • Warnings: --max-warnings=0 so rules landing at warn level cannot pass CI silently; this also fails on unused eslint-disable directives, keeping the remaining src/ directives honest.
  • Mechanical fixes: import/order in five files, one prefer-const (mockPbjs declaration merged with its single assignment).

No workflow changes: format.yml already runs npm run lint; the script now covers the whole package.

Verification

  • npx eslint . --max-warnings=0 exits 0
  • npm run format clean
  • npx vitest run: 488 passed, 0 failed (32 files)
  • npx tsc --noEmit: no new errors vs main (50 -> 12)

The lint script only covered src/**, so eslint errors in test files and
the Node build scripts were invisible to the CI lint gate in format.yml.
Widen the script to eslint . and make the widened surface clean:

- Disable @typescript-eslint/no-explicit-any for test files — tests
  routinely poke at private state and mock boundaries via any, and the
  rule produced 249 errors there; drop the per-line disables it makes
  redundant.
- Declare Node globals for the .mjs build scripts and Node-run test
  harnesses via the globals package.
- Honor the underscore convention for intentionally unused parameters
  in @typescript-eslint/no-unused-vars.
- Fix the remaining import/order and prefer-const violations.
@aram356 aram356 self-assigned this Jul 31, 2026
@aram356
aram356 requested review from ChristianPavilonis and prk-Jr and removed request for prk-Jr July 31, 2026 05:22

@prk-Jr prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Widens the JS lint gate from src/** to the whole package and cleans the newly-covered surface. Behavior-neutral: no production src/ file is touched — every code edit is a comment removal, an import blank line, or the letconst restructure in the prebid mock. Verified locally: npx eslint . exits 0 (1.7s), and the dist ignore holds (dropped a deliberately-broken probe file into dist/, still exit 0). Approving; the findings below are all non-blocking.

Non-blocking

🤔 thinking

  • Blanket no-explicit-any off for test/**: defensible at 249/280 and it matches the existing file convention, but it also covers shared helpers/fixtures where types are cheap, and new any in tests now lands with no signal. (eslint.config.js:53)
  • .mjs gets a weaker gate than "full package" implies: project rules are scoped to **/*.ts(x), so the newly-covered build scripts only get js.configs.recommended + tseslint defaults. (eslint.config.js:46)

♻️ refactor

  • The ^_ convention does not reach .mjs: the no-unused-vars options live in the **/*.ts block, but the rule is active with default options on .mjs. (eslint.config.js:38)

🌱 seedling

  • lint has no --max-warnings=0: 0 warnings today, so no live gap — but any future 'warn'-level rule passes CI silently, which is the same class of hole this PR exists to close. (package.json:13)

📌 out of scope

  • A second TypeScript package is still unlinted: crates/trusted-server-integration-tests/browser/ (Playwright specs, helpers/, global-setup.ts, playwright.config.ts) has no lint script and no format.yml job. If the intent of #985 is "no unlinted TS in CI", that is the remaining gap — worth a follow-up issue rather than scope creep here.

⛏ nitpick

  • Stray blank lines where disable directives were removed: ten spots in test/integrations/gpt/ad_init.test.ts. Seven (1408, 1491, 1523, 1559, 1759, 1820, 1895) read fine as paragraph breaks; three inside two-line arrow bodies (1344, 1374, 1731) read oddly. Prettier will not collapse either.

👍 praise

  • Forward-reference comment in the prebid mock: the letconst restructure could read as a TDZ hazard; the comment states exactly why it is not. (test/integrations/prebid/index.test.ts:25)
  • Behavior-neutral diff: keeping the gate widening free of any production change makes "widen the gate, then clean the surface" easy to trust in review.

CI Status

All 19 checks pass on 959f4b9.

  • cargo fmt / clippy: PASS
  • rust tests (fastly, axum, cloudflare, spin, parity, CLI): PASS
  • vitest: PASS
  • format-typescript (eslint + prettier): PASS
  • format-docs: PASS
  • integration + browser integration tests: PASS

Comment thread crates/trusted-server-js/lib/eslint.config.js
Comment thread crates/trusted-server-js/lib/eslint.config.js
Comment thread crates/trusted-server-js/lib/eslint.config.js Outdated
Comment thread crates/trusted-server-js/lib/package.json Outdated
Comment thread crates/trusted-server-js/lib/test/integrations/gpt/ad_init.test.ts Outdated
Comment thread crates/trusted-server-js/lib/test/integrations/prebid/index.test.ts

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Reviewed the lint-surface expansion against main. The change is low risk and behavior-neutral overall; all CI checks and clean-checkout JS validation pass. I found one medium-severity lint-environment gap, noted inline.

Comment thread crates/trusted-server-js/lib/eslint.config.js Outdated
aram356 added 6 commits July 31, 2026 12:52
…fail on warnings

- Use globals.nodeBuiltin instead of globals.node for .mjs files so
  CommonJS-only names (__dirname, require, module) still fail no-undef
  in ES modules
- Lift the no-unused-vars underscore ignore patterns to a top-level
  rules block so the convention also covers the .mjs build scripts,
  where tseslint recommended otherwise applies pattern-less defaults
- Add --max-warnings=0 to the lint scripts so rules landing at 'warn'
  cannot pass CI silently
- Collapse leftover blank lines in two-line arrow bodies in
  ad_init.test.ts from the eslint-disable directive removals
Remove the test/** rule-off block so the rule stays live for new test
files, and handle the 255 existing occurrences by tier:

- File-level eslint-disable in the six files where `any` is structural
  to the mocking approach (prebid/index, gpt/ad_init, gpt/index,
  core/request, core/index, didomi/index — 246 occurrences)
- Real types where they were cheap: fetch stubs cast via
  `as unknown as typeof fetch` (auction, click, proxy_sign), `as
  AdUnit` with a type-only import in registry, and the config cast
  dropped entirely ('info' is already in the logLevel union)
- A scoped eslint-disable-next-line for gpt_bootstrap's AnyRecord alias

ESLint 9 reports unused disable directives as warnings by default, so
with --max-warnings=0 any directive made redundant later fails CI.
Replace the six file-level eslint-disable headers and the one scoped
directive with actual types, eliminating all 247 remaining any usages
in test/. The rule now applies to the whole test tree with no
exceptions.

- Window mocks: Omit<Window, 'tsjs'> intersections with Partial<TsjsApi>
  from src/core/types, replacing the any-typed tsjs surfaces; this was
  the real fix for the intersection problem an old ad_init comment used
  to justify any
- Prebid: structural TestPbjs/TestAdUnit/TestBid types over the hoisted
  mock; ~50 argument-site casts collapse into declaration-site
  as TestPbjs casts (a plain annotation cannot compile against Prebid's
  contravariant requestBids signature)
- core/index: drop the local Window.tsjs declare-global that conflicted
  with the src declaration
- didomi: local TestDidomiConfig mirroring the non-exported src type
- Many casts turned out gratuitous (untyped vi.fn() already assigns to
  typeof fetch; ad-unit literals already satisfy AdUnit) and were
  deleted outright

One deliberate double-cast survives: prebid feeds a non-array bids
value through as unknown as TestAdUnit[] to prove the shim normalizes
malformed input.

Type-only change: no statement, assertion, or mock behavior touched.
Side effect: repo tsc --noEmit errors drop from 50 to 12, all remaining
ones pre-existing in files this PR does not touch.
Replace the four remaining em dashes in comments with plain wording,
and hoist the repeated NestedServerParams/NestedBrowserParams casts in
the nested-isolation test into refreshServerParams/refreshBrowserParams
locals, named apart from the publisher-side serverParams/browserParams
seeded earlier in the same test.
The merge from main brought the new gpt_diagnostics test (#974), which
predates this PR's test-wide no-explicit-any enforcement and failed the
widened CI lint on the merge result. The stub only stores and relays
event payloads without inspecting them, so unknown is the accurate
parameter type; the src GptPubAdsService declares addEventListener as a
method, whose bivariant parameter check accepts the narrowing.
@aram356
aram356 merged commit 9414734 into main Aug 1, 2026
19 checks passed
@aram356
aram356 deleted the ci-lint-js-tests branch August 1, 2026 05:04
aram356 added a commit that referenced this pull request Aug 1, 2026
PR #916 was squash-merged into main and this PR was retargeted to main, so
the previously merged base content re-conflicted without shared history.
Every conflicted region resolves to this branch's version, which already
contains the base content plus this PR's changes; the only main-side delta
adopted inside a conflicted file is the pub(crate) visibility on
process_auction_creative. Main's GPT diagnostics overlay (#974) and lint
scope (#984) changes merged cleanly.
aram356 added a commit that referenced this pull request Aug 1, 2026
…c/july

Adopts main's finalized #974 GPT diagnostics (standalone ts_console-gated
module, finalize_response wiring, updated overlay/store/badges/binding)
over rc's earlier absorbed copy, and #984's whole-package eslint gate
(eslint . --max-warnings=0). Keeps rc-only content where the two lint
passes collided: the #963/#967/#988 test rewrites in the prebid, APS,
request, and ad_init suites, the GptSlotHandoff type, and the
unexpected-origin-304 guard alongside main's diagnostics finalize call
in publisher.rs.
aram356 added a commit that referenced this pull request Aug 1, 2026
origin/worktree-config-audit merged main only through #937; this branch
already merged main through #984 plus the config-validation review fixes.
Keep this branch's superset resolution (favor ours); the remote merge adds
no content this branch lacks.
aram356 added a commit that referenced this pull request Aug 1, 2026
CodeQL flagged the click guard's navigation and href-persist sinks: the
inputs are creative-controlled DOM attributes, so a javascript: value in
data-tsclick or href could reach location.href or be written back as an
anchor href. Resolve every candidate URL against the pinned trusted base and
require an http(s) scheme before navigating or persisting, failing closed
otherwise. Also replace an as-any cast in the new click test now that main
lints the full JS package (#984).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI lint gate misses JS test files and Node build scripts

3 participants