Skip to content

Conversation

@birkskyum
Copy link
Member

@birkskyum birkskyum commented Oct 26, 2025

@schiller-manuel , i found that these tests had toBeTruthy that weren't called, and they break if they are enabled, which is demonstrated here:

I think they should just be checking expect(request.redirectedFrom()).toBeNull()

Summary by CodeRabbit

  • Tests
    • Simplified redirect verification assertions in end-to-end tests across framework implementations to improve test clarity and maintainability.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2025

Walkthrough

Replace indirect redirect comparison in two end-to-end tests with a direct absence check (asserting redirectedFrom() is null), and remove a trailing slash before query parameters in one test URL.

Changes

Cohort / File(s) Summary
Redirect absence assertions
e2e/react-start/basic/tests/search-params.spec.ts, e2e/solid-start/basic/tests/search-params.spec.ts
Simplified the no-redirect assertion to check redirectedFrom() is null instead of traversing redirectedFrom()?.redirectedTo() === request.
Test URL normalization
e2e/solid-start/basic/tests/search-params.spec.ts
Removed a trailing slash before query parameters in the default search params test URL (changed /search-params/default/?default=d2 to /search-params/default?default=d2).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Test as Test Runner
  participant Browser as Browser/Request
  participant Page as Page under test

  rect rgb(240,248,255)
    Note right of Test: Issue request to Page
    Test->>Browser: navigate(request)
    Browser->>Page: load request
  end

  rect rgb(245,255,240)
    Note right of Test: Check for redirect origin
    Browser->>Test: redirectedFrom()
    alt redirectedFrom() is null
      Note right of Test: New check — assert no redirect origin
      Test->>Test: expect(redirectedFrom()).toBeNull()
    else redirectedFrom() not null
      Note right of Test: Previous logic compared origin→target equality
      Test->>Test: expect(redirectedFrom()?.redirectedTo() === request)
    end
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Small, focused changes in two test files.
  • Review attention items:
    • Confirm the new redirectedFrom() null-check matches intended test semantics.
    • Verify the adjusted URL (removed slash) doesn't change test routing expectations.

Poem

🐰 A hop, a check, the redirect's gone,

I sniff the path at early dawn.
No trailing slash, the query's neat,
Tests now skip a sideways beat.
A tiny hop — the code's complete. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "test(start): fix search-params tests" directly relates to the main changes in the changeset. The modifications are exclusively in search-params test files within the start packages (react-start and solid-start), where assertions are being fixed to properly verify the absence of redirects. The title accurately and concisely summarizes the primary change—fixing search-params tests—which aligns with the PR's stated objective of resolving broken test assertions that weren't being called.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch add-missing-call-to-toBeTruthy

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1798211 and 16bd1af.

📒 Files selected for processing (2)
  • e2e/react-start/basic/tests/search-params.spec.ts (1 hunks)
  • e2e/solid-start/basic/tests/search-params.spec.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • e2e/solid-start/basic/tests/search-params.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • e2e/react-start/basic/tests/search-params.spec.ts
e2e/**

📄 CodeRabbit inference engine (AGENTS.md)

Store end-to-end tests under the e2e/ directory

Files:

  • e2e/react-start/basic/tests/search-params.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Preview
🔇 Additional comments (1)
e2e/react-start/basic/tests/search-params.spec.ts (1)

22-22: Good fix - direct null assertion is clearer.

The change from an indirect comparison to expect(request.redirectedFrom()).toBeNull() directly asserts the absence of a redirect, making the test more explicit and fixing the issue where the assertion wasn't being called.


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 and usage tips.

@nx-cloud
Copy link

nx-cloud bot commented Oct 26, 2025

View your CI Pipeline Execution ↗ for commit 16bd1af

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded <1s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 3s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-27 16:00:21 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 26, 2025

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5643

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@5643

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@5643

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5643

@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/nitro-v2-vite-plugin@5643

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@5643

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@5643

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@5643

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@5643

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@5643

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@5643

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@5643

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@5643

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@5643

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@5643

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@5643

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@5643

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@5643

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5643

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@5643

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@5643

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@5643

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@5643

@tanstack/solid-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-ssr-query@5643

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@5643

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@5643

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@5643

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@5643

@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@5643

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@5643

@tanstack/start-static-server-functions

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-static-server-functions@5643

@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@5643

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5643

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@5643

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@5643

commit: 16bd1af

@birkskyum
Copy link
Member Author

birkskyum commented Oct 26, 2025

@schiller-manuel , i found that these tests had toBeTruthy that weren't called, and they break if they are enabled. I think they should just be checking expect(request.redirectedFrom()).toBeNull()

@birkskyum birkskyum changed the title test(start): add missing call in search-params test test(react-start): add missing call in search-params test Oct 26, 2025
@birkskyum birkskyum changed the title test(react-start): add missing call in search-params test test(react-start, solid-start): add missing call in search-params test Oct 27, 2025
@birkskyum birkskyum changed the title test(react-start, solid-start): add missing call in search-params test test(start): fix search-params tests Oct 27, 2025
@birkskyum birkskyum force-pushed the add-missing-call-to-toBeTruthy branch from 1798211 to 16bd1af Compare October 27, 2025 15:59
@birkskyum birkskyum merged commit 3eeeb88 into main Oct 27, 2025
6 checks passed
@birkskyum birkskyum deleted the add-missing-call-to-toBeTruthy branch October 27, 2025 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants