fix(router-tests): fix connectrpc tests and add to CI

[asoorm]

Summary

• Fix test service directory paths from samples/services → testdata/services (directory was renamed)
• Fix HTTP 404 mapping assertion: CodeUnimplemented not CodeNotFound per Connect RPC spec
• Add ./connectrpc to the CI integration test matrix so these tests run on every PR

Fixes ENG-8984

Test plan

• All 21 connectrpc tests pass locally with race detector enabled
• CI pipeline runs the new ./connectrpc matrix entry successfully

Summary by CodeRabbit

• Tests
  • Expanded the CI test matrix to include additional RPC protocol test targets for broader coverage
  • Refined test assertions around HTTP error-code mapping to ensure correct error classification
  • Reorganized test data layout for improved maintainability and clearer test organization

[coderabbitai]

Walkthrough

Adds connectrpc to CI integration tests, updates an HTTP 404 mapping test to expect CodeUnimplemented, and switches test service directory references from samples to testdata across connectrpc tests.

Changes

Cohort / File(s)	Summary
Workflow Configuration .github/workflows/router-ci.yaml	Added connectrpc to the integration_test matrix test_target list.
ConnectRPC Tests — Error Mapping router-tests/connectrpc/connectrpc_client_test.go	Changed test expectation: HTTP 404 now asserted as CodeUnimplemented (renamed test accordingly).
ConnectRPC Tests — Service Paths router-tests/connectrpc/connectrpc_server_lifecycle_test.go, router-tests/connectrpc/connectrpc_test_helpers.go	Updated ServicesDir path from ../../router/pkg/connectrpc/samples/services to ../../router/pkg/connectrpc/testdata/services.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: fixing connectrpc tests and adding them to CI, which directly corresponds to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Router-nonroot image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-4bc6e992cd63abf9a7df2cda5358327ea052b035-nonroot

[coderabbitai]

🧹 Nitpick comments (2)

router-tests/connectrpc/connectrpc_client_test.go (1)

28-28: Redundant defer ts.Close().

NewTestConnectRPCServer already registers t.Cleanup(func() { ts.Close() }), and Close() has an idempotency guard (cleanupDone), so the explicit defer ts.Close() on line 28 is harmless but unnecessary. The other error-handling tests in this same file omit it consistently.

🔧 Suggested cleanup

 	ts := NewTestConnectRPCServer(t, ConnectRPCServerOptions{
 		GraphQLHandler: EmployeeGraphQLHandler(),
 	})
-	defer ts.Close()
-	
-	err := ts.Start()
+	err := ts.Start()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@router-tests/connectrpc/connectrpc_client_test.go` at line 28, Remove the
redundant explicit defer call to ts.Close() in the test after calling
NewTestConnectRPCServer; NewTestConnectRPCServer already schedules
t.Cleanup(func() { ts.Close() }) and Close() is idempotent via cleanupDone, so
simply delete the line containing defer ts.Close() to keep tests consistent with
the other error-handling tests that rely on t.Cleanup.

.github/workflows/router-ci.yaml (1)

199-204: LGTM — consider a dedicated lighter job for ./connectrpc later.

The matrix addition is correct. However, the connectrpc tests (reviewed below) rely only on a mock HTTP server and an in-process ConnectRPC server — they have no dependency on nats, redis cluster, or kafka. Every PR will now start an extra ubuntu-latest-l runner, initialize a 3-node Redis cluster (up to ~60 s wait), and boot nats + kafka, all unused.

This is fine for now given the simplicity of a single shared job, but worth revisiting if CI times become a concern: a minimal job without the service stack would suffice for ./connectrpc.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/router-ci.yaml around lines 199 - 204, Current change adds
'./connectrpc' to the heavy test matrix which unnecessarily starts services;
remove './connectrpc' from the existing matrix and add a new lightweight CI job
(e.g., "connectrpc-tests") that runs only the './connectrpc' path, uses an
ubuntu-latest runner, skips starting Redis/NATS/Kafka services, and runs the
same test commands used in the main job (ensure the step that executes the tests
targets the ./connectrpc directory); update the workflow matrix/paths to exclude
'./connectrpc' from the heavy job and create the new job with minimal service
setup to keep CI times down.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.github/workflows/router-ci.yaml:
- Around line 199-204: Current change adds './connectrpc' to the heavy test
matrix which unnecessarily starts services; remove './connectrpc' from the
existing matrix and add a new lightweight CI job (e.g., "connectrpc-tests") that
runs only the './connectrpc' path, uses an ubuntu-latest runner, skips starting
Redis/NATS/Kafka services, and runs the same test commands used in the main job
(ensure the step that executes the tests targets the ./connectrpc directory);
update the workflow matrix/paths to exclude './connectrpc' from the heavy job
and create the new job with minimal service setup to keep CI times down.

In `@router-tests/connectrpc/connectrpc_client_test.go`:
- Line 28: Remove the redundant explicit defer call to ts.Close() in the test
after calling NewTestConnectRPCServer; NewTestConnectRPCServer already schedules
t.Cleanup(func() { ts.Close() }) and Close() is idempotent via cleanupDone, so
simply delete the line containing defer ts.Close() to keep tests consistent with
the other error-handling tests that rely on t.Cleanup.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.96%. Comparing base (c8a6a5d) to head (78c8d1c).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2542       +/-   ##
===========================================
- Coverage   61.69%   44.96%   -16.73%     
===========================================
  Files         239      239               
  Lines       25423    25423               
===========================================
- Hits        15685    11432     -4253     
- Misses       8418    12812     +4394     
+ Partials     1320     1179      -141     

see 117 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[StarpTech]

LGTM

[coderabbitai]

🧹 Nitpick comments (1)

.github/workflows/router-ci.yaml (1)

192-204: Operational note: connectrpc matrix leg spins up services it doesn't use.

Every integration_test matrix entry — including ./connectrpc — waits for a Redis Cluster to form, configures cluster ACLs, and starts NATS and Kafka. Connect RPC tests are pure HTTP/gRPC protocol tests and almost certainly require none of those services. This adds roughly 2–3 minutes of service-bootstrap overhead to every connectrpc run on an expensive large runner.

Consider either:

1. Grouping ./connectrpc alongside another lightweight target in a single matrix entry (e.g., './telemetry ./connectrpc'), or
2. Splitting it into a separate, slimmer job without the Redis Cluster / NATS / Kafka service block — similar to how build_test_fork is kept lightweight.

Neither change is blocking, but the waste compounds on every PR touching router-tests.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/router-ci.yaml around lines 192 - 204, The
integration_test matrix currently runs a heavy service bootstrap for every
test_target including './connectrpc', causing unnecessary Redis/NATS/Kafka
startup overhead; update the CI by either grouping './connectrpc' with a
lightweight target (e.g., change the matrix entry to include './telemetry
./connectrpc' so it shares a single lightweight matrix leg) or create a separate
job (copy of integration_test named e.g., integration_test_connectrpc) that
omits the Redis Cluster / ACL / NATS / Kafka service block (mirror how
build_test_fork is kept lightweight) so the connectrpc runs without the
expensive service bootstrap.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In @.github/workflows/router-ci.yaml:
- Around line 192-204: The integration_test matrix currently runs a heavy
service bootstrap for every test_target including './connectrpc', causing
unnecessary Redis/NATS/Kafka startup overhead; update the CI by either grouping
'./connectrpc' with a lightweight target (e.g., change the matrix entry to
include './telemetry ./connectrpc' so it shares a single lightweight matrix leg)
or create a separate job (copy of integration_test named e.g.,
integration_test_connectrpc) that omits the Redis Cluster / ACL / NATS / Kafka
service block (mirror how build_test_fork is kept lightweight) so the connectrpc
runs without the expensive service bootstrap.