Skip to content

test: remove TS constant self-equals tautologies (audit T-L1) - #1475

Merged
DeliciousBuding merged 1 commit into
masterfrom
test/ts-self-equals-cleanup
Jul 29, 2026
Merged

test: remove TS constant self-equals tautologies (audit T-L1)#1475
DeliciousBuding merged 1 commit into
masterfrom
test/ts-self-equals-cleanup

Conversation

@DeliciousBuding

@DeliciousBuding DeliciousBuding commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Audit T-L1 — TS constant self-equals tautology cleanup

Audit finding T-L1 (comprehensive audit 2026-07-29) flagged "constants self-equals" anti-pattern: assertions like expect(x).toBe(x) / assert.equal(x, x) where both sides are the same constant/variable, which assert nothing useful (a constant equals itself trivially). The audit estimated ~52 instances across the TS side; this PR does the actual enumeration and cleanup.

Scope

  • TS test files only under app/ (*.test.ts, *.test.tsx, *.spec.ts, *.spec.tsx).
  • Go tests untouched (per task brief; the ~34 Go occurrences in assemble_test.go are mostly real field-vs-value comparisons).
  • Non-test files untouched.

Step 1 — Enumeration result

Exhaustive scan via recursive parser (handles nested parens, multi-line, string literals, chai/vitest/assert styles, all matchers including .not., .resolves., .poll.):

  • expect(X).toBe(X) self-equals (same identifier on both sides): 1
  • expect(X).toEqual(X) self-equals: 0
  • expect(X).toStrictEqual(X): 0
  • assert.equal(X, X) / assert.strictEqual(X, X) / assert.deepEqual(X, X): 0
  • expect(LITERAL).toBe(SAME_LITERAL) (different quote styles, e.g. 'OK' vs "OK"): 0
  • Other matchers (toHaveLength, toMatch, toContain) with self-equals: 0

Actual TS-side occurrences: 1 (the audit's ~52 estimate was overstated — most flagged candidates were real expect(computed).toBe(expected) comparisons with two distinct variables).

Step 2 — Judgment & fix

File: app/desktop/src/__e2e__/oidc-login.spec.ts:123

Before:

} else {
  // Login button may not be visible if already on a different page;
  // this is fine — we just verify the app loads
  expect(true).toBe(true);
}

After:

} else {
  // Login button may not be visible if already on a different page;
  // this is fine — we just verify the app rendered without crashing.
  await expect(page.locator('#root')).toBeAttached();
  await expect(page.locator('#root > *').first()).toBeVisible();
}

Replaced the vacuous assertion with two meaningful checks: (1) #root is attached to the DOM (page mounted), (2) at least one child element rendered and is visible (React hydrated). Matches the pattern used in app/desktop/src/__e2e__/smoke.spec.ts ('app loads without crash'). Per the task brief's conservative default: replaced with a stronger assertion rather than deleting the test branch.

False positives kept (sample of common ones)

  • expect(agent1.id).toBe(agent2.id) (pipeline-integration.test.ts:749-750) — compares two distinct agent ids to verify idempotency/dedup. KEEP.
  • expect(c1).toBe(c2) (desktop hubAuth.test.ts:96) — compares two PKCE challenge outputs from the same input to verify determinism. KEEP.
  • expect(typesA).toEqual(typesB), expect(zhKeys).toEqual(enKeys) — cross-variable comparisons. KEEP.
  • expect(lower.length).toBe(upper.length) (mobile-rn threads.test.ts:203) — compares two different arrays' lengths. KEEP.
  • expect(res.status).toBe(200), expect(row.status).toBe('ok') etc. — value-vs-literal, real assertions. KEEP.

Packages touched

  • agenthub-desktop (1 file: app/desktop/src/__e2e__/oidc-login.spec.ts)

Step 4 — Verification

  • Touched file is a Playwright e2e spec (__e2e__), excluded from desktop vitest config (vitest.config.ts test.include), so it does not run under the vitest suite. Typecheck via tsc --noEmit -p tsconfig.json --ignoreDeprecations 6.0 reports 3964 baseline errors both with and without this change (unchanged), and zero new errors in the touched file.
  • Full desktop vitest suite: 1515 passed / 8 failed / 34 failed files — identical to baseline without this change (the 8 failures are pre-existing integration tests requiring a live Edge server; unrelated to this edit).
  • Playwright e2e requires a running dev server (pnpm dev); not executed here since the change is a pure assertion replacement using the same locator pattern already present in smoke.spec.ts.

Summary

  • Instances found: 1
  • Removed/replaced: 1 (replaced vacuous expect(true).toBe(true) with two real DOM-render assertions)
  • False positives kept with reasons: documented above
  • Files changed: 1
  • Test result: no regression (baseline == patched)

Summary by CodeRabbit

  • Tests
    • Improved end-to-end login test validation by confirming the application renders visible content when the login button is unavailable.

Copilot AI review requested due to automatic review settings July 29, 2026 16:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

OIDC login E2E validation

Layer / File(s) Summary
Validate fallback rendering
app/desktop/src/__e2e__/oidc-login.spec.ts
The fallback branch checks that #root is attached and that its first child is visible instead of using a placeholder assertion.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: removing a tautological test assertion during the audit.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/ts-self-equals-cleanup

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/desktop/src/__e2e__/oidc-login.spec.ts`:
- Around line 122-124: Update the assertions in the OIDC login test to verify
the expected AppShell or fallback-specific state rather than merely checking
that any `#root` child is visible. Use a stable selector or accessible marker
corresponding to the intended rendered application state, while retaining the
root attachment check.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 05a360c2-a37b-4b94-9586-8bfcf047176f

📥 Commits

Reviewing files that changed from the base of the PR and between 53ce53a and c265a1a.

📒 Files selected for processing (1)
  • app/desktop/src/__e2e__/oidc-login.spec.ts

Comment on lines +122 to +124
// this is fine — we just verify the app rendered without crashing.
await expect(page.locator('#root')).toBeAttached();
await expect(page.locator('#root > *').first()).toBeVisible();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the expected app state, not any visible root child.

Because AppShell is wrapped by ErrorBoundary in app/desktop/src/main.tsx, an error fallback could satisfy #root > * visibility while the application has crashed. Use a stable selector or accessible state specific to the expected fallback/app-shell rendering.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/desktop/src/__e2e__/oidc-login.spec.ts` around lines 122 - 124, Update
the assertions in the OIDC login test to verify the expected AppShell or
fallback-specific state rather than merely checking that any `#root` child is
visible. Use a stable selector or accessible marker corresponding to the
intended rendered application state, while retaining the root attachment check.

@DeliciousBuding
DeliciousBuding merged commit 5484f51 into master Jul 29, 2026
20 checks passed
@DeliciousBuding
DeliciousBuding deleted the test/ts-self-equals-cleanup branch July 29, 2026 16:44
DeliciousBuding added a commit that referenced this pull request Jul 29, 2026
Lane A #1476: TriggerAgentTask 成功路径 + 4 错误路径 + #1430 TurnInProgress
409 二次触发 gate 直测(320 行,T-M1 覆盖缺口闭环)。
Lane B #1475: 全仓 TS 自等扫仅 1 处真 tautology(oidc-login
expect(true).toBe(true)),审计「52 处」高估→澄清并修正为 DOM 渲染断言。
综合审计 top10 全部落地 10/10。master tip 31c482c
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.

2 participants