Skip to content

SonarCloud: JavaScript/TypeScript code quality #509

Closed

Description

@don-petry

SonarCloud: JavaScript/TypeScript code quality

17 open SonarCloud finding(s) in petry-projects/google-app-scripts, worst severity MAJOR. Generated by the org SonarCloud Audit.

View in SonarCloud →

Findings

Rule Count Representative message
javascript:S8754 14 Rename this test title to make it unique within the suite.
javascript:S8786 3 Simplify this regular expression to reduce its runtime, as it has super-linear performance

Affected files

  • src/calendar-to-sheets/tests/index.test.js (14)
  • src/gas-utils.js (3)

Priority

Labeled priority:major — mapped from this bucket's worst SonarCloud severity (MAJOR).

Acceptance

  • All findings in this workstream resolved to zero in SonarCloud
  • No behavior change; CI green
  • Real fixes (no blanket NOSONAR unless a confirmed false positive, noted inline)

Idempotent issue — updated automatically each audit run; auto-closed when the finding count reaches zero.

Activity

  1. don-petry commented on Aug 4, 2026

    @don-petry
    CollaboratorAuthor

    Dev-Lead Implementation Plan

    Issue: #509 — SonarCloud: JavaScript/TypeScript code quality

    Scope

    Resolve 17 SonarCloud findings: 14 non-unique test titles (javascript:S8754) in the calendar-to-sheets test suite, and 3 super-linear regexes (javascript:S8786) in src/gas-utils.js — with no behavior change.

    Investigation summary

    • S8754 (14): All 14 non-unique titles are redundant copy-paste duplicates within the same suite (verified via diff): a 7-test block (fullResync…, tail merge…, …logs warning…) duplicated verbatim, a 3-test getConfigs/empty-SYNC_CONFIGS block (one test differs only in assertion style, e.g. toHaveLength(1) vs .length).toBe(1) — functionally identical), plus 4 standalone duplicated tests. Fix: remove the redundant second occurrences (real fix, not cosmetic renaming). Coverage is provably unchanged since each removed test is identical/equivalent to a retained one.
    • S8786 (3): The three /^\s*…/m patterns are super-linear because \s matches newlines, so under /m the leading \s* backtracks across every line anchor → O(n²). Fix: scope the leading whitespace to horizontal-only (^[^\S\r\n]*). Internal \s connectors are left intact so the cross-line From:/Sent: match is preserved.

    Implementation Checklist

    • src/calendar-to-sheets/tests/index.test.js — remove 14 duplicate tests (5 contiguous blocks)
    • src/gas-utils.js — rewrite 3 leading ^\s* anchors as ^[^\S\r\n]*

    Tests to Write

    • No new tests — behavior-preserving cleanup/refactor. Existing suites (index.test.js, gas-utils.test.js) are the regression guard. ReDoS fix additionally verified with a timing check (O(n²) → O(n)).

    Test Command

    npx jest --runInBand (+ npx jest --coverage and node scripts/check-coverage.js)

  2. don-petry commented on Aug 4, 2026

    @don-petry
    CollaboratorAuthor

    Dev-Lead: Implementation Complete

    Plan Execution

    • Removed 14 redundant duplicate tests (javascript:S8754) from src/calendar-to-sheets/tests/index.test.js
    • Fixed 3 super-linear regexes (javascript:S8786) in src/gas-utils.js
    • Full suite + coverage thresholds green; repo lint (dev-lead-lint.sh), prettier, and eslint clean

    What changed & why

    S8754 — non-unique test titles (14): every flagged title was a verbatim (or assertion-style-only) copy-paste duplicate within the same Jest suite. Removed the redundant second occurrences rather than renaming, since keeping two identical tests under different names is not a real fix. Each removed test is identical/equivalent to a retained one, so coverage is unchanged.

    S8786 — super-linear regexes (3): the three /^\s*…/m header patterns backtracked in O(n²) because \s matches newlines, so under /m the leading \s* scans across every line anchor. Scoped the leading anchor to horizontal whitespace (^[^\S\r\n]*). Internal \s connectors were left untouched so the cross-line From:/Sent: reply-header match still works. Verified O(n²) → O(n): old pattern 3.8→14→56 ms at n=2k/4k/8k; new pattern flat ~0.04 ms.

    Test Results

    
    Test Suites: 14 passed, 14 total
    Tests:       378 passed, 378 total
    Snapshots:   0 total
    Time:        3.287 s, estimated 6 s
    Ran all test suites.
    

    Coverage

    
    ✅ statements     : 98.71% (1146/1161) [required: 95%]
    ✅ branches       : 90.19% (625/693) [required: 85%]
    ✅ functions      : 97.50% (117/120) [required: 95%]
    ✅ lines          : 100% (0/0) [required: 100%]
    
    ✅ All coverage metrics meet requirements.
    

    Files Changed

    • src/calendar-to-sheets/tests/index.test.js: removed 14 duplicate tests (434 lines) across 5 contiguous blocks; all titles now unique per suite.
    • src/gas-utils.js: 3 regex leading anchors ^\s* → ^[^\S\r\n]*.

    Notes

    • Behavior-preserving change, so Phase 2 (test-first) was skipped per the documented exception; the existing index.test.js and gas-utils.test.js suites act as the regression guard. Test count went 392 → 378 (the 14 removed duplicates); coverage unchanged (lines 100%, all thresholds met).
    • The 22 eslint warnings from npm run check are pre-existing unused-var warnings in unrelated gmail-to-drive-by-labels test files — not touched by this change.
  3. added a commit that references this issue on Aug 4, 2026
  4. added a commit that references this issue on Aug 7, 2026
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

    dev-leadFor dev-lead agent pickuppriority:majorSonarCloud MAJOR severitysonarcloud-auditSonarCloud audit finding (idempotent marker)

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions