Skip to content

fix: remove wrong test case#2453

Merged
alepane21 merged 3 commits intomainfrom
ale/eng-8270-router-potential-issue-with-redis-clusterurlslice
Jan 15, 2026
Merged

fix: remove wrong test case#2453
alepane21 merged 3 commits intomainfrom
ale/eng-8270-router-potential-issue-with-redis-clusterurlslice

Conversation

@alepane21
Copy link
Copy Markdown
Contributor

@alepane21 alepane21 commented Jan 14, 2026

This test was left there because failing after the last refactor, waiting for a check. I've checked and find out that:

  • it was checking something that is not happening
  • it is a behaviour not documented and useless
    So I just removed the test.

Summary by CodeRabbit

  • Tests
    • Removed a cluster-mode test case that covered authentication fallback between multiple URLs, slightly reducing related test coverage.
    • No other test cases or public interfaces were changed; remaining tests and functionality are unchanged.

✏️ Tip: You can customize this high-level summary in your review settings.

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/cosmo-docs.
  • I have read the Contributors Guide.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jan 14, 2026

Walkthrough

Removes a cluster-mode test case from the rate limit test suite: the test "should successfully use auth from later url if no auth in first urls" and its surrounding commented block were deleted, leaving no other functional changes.

Changes

Cohort / File(s) Summary
Rate Limit Test Suite
router-tests/ratelimit_test.go
Deleted a commented/disabled cluster-mode test case that verified using auth from a later URL when earlier URLs lacked auth; no other changes made

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ⚠️ Warning The PR title claims to 'remove wrong test case' but the PR objectives indicate the intent was to 're-enable test for auth from later URL', creating a significant disconnect between the stated title and the PR's documented purpose. Align the PR title with the actual change: either correct it to 'fix: remove wrong test case' if removal is the intent, or update to reflect the re-enabling objective if that was the true goal. The commit message confirms test removal was the actual change made.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches
  • 📝 Generate docstrings

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

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 14, 2026

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-bb45e432f27141e8a5471e9c4a255e9c80bdcdec

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@router-tests/ratelimit_test.go`:
- Around line 711-714: The test name "should successfully use auth from later
url if no auth in first urls" is misleading because the cluster implementation
(rediscloser.go) only uses credentials from the first URL and strips creds from
secondary URLs; update the table entry that defines name and clusterUrlSlice
(the case with []string{"redis://localhost:7003", "rediss://localhost:7001",
"rediss://cosmo:test@localhost:7002"}) to either (a) rename the test to reflect
that auth comes from explicit Username/Password or from the first URL (e.g.,
"uses explicit Username/Password when secondary URLs contain credentials") or
(b) remove the case entirely if you intended to test extracting auth from later
URLs but that behavior is unsupported; ensure the test assertions match the
actual behavior (i.e., credentials are provided via Username/Password, not
parsed from later URLs).
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08bcfb5 and 5474f89.

📒 Files selected for processing (1)
  • router-tests/ratelimit_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.

Applied to files:

  • router-tests/ratelimit_test.go
📚 Learning: 2026-01-06T12:37:21.521Z
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.

Applied to files:

  • router-tests/ratelimit_test.go
⏰ 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). (11)
  • GitHub Check: build_test
  • GitHub Check: build_test
  • GitHub Check: build_push_image
  • GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
  • GitHub Check: image_scan
  • GitHub Check: image_scan (nonroot)
  • GitHub Check: integration_test (./events)
  • GitHub Check: build_push_image (nonroot)
  • GitHub Check: integration_test (./telemetry)
  • GitHub Check: Analyze (javascript-typescript)
  • GitHub Check: Analyze (go)

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.37%. Comparing base (24d0709) to head (f7856f6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2453       +/-   ##
===========================================
+ Coverage   37.66%   61.37%   +23.71%     
===========================================
  Files         769      229      -540     
  Lines      114353    23839    -90514     
  Branches     7869        0     -7869     
===========================================
- Hits        43066    14632    -28434     
+ Misses      70928     7971    -62957     
- Partials      359     1236      +877     

see 998 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.

…ppening and not documented, so no need for it at all
@alepane21 alepane21 changed the title chore: reenable test for auth from later URL in cluster slice fix: remove wrong test case Jan 15, 2026
Copy link
Copy Markdown
Member

@Aenimus Aenimus left a comment

Choose a reason for hiding this comment

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

LGTM!

@alepane21 alepane21 merged commit ae35b80 into main Jan 15, 2026
21 checks passed
@alepane21 alepane21 deleted the ale/eng-8270-router-potential-issue-with-redis-clusterurlslice branch January 15, 2026 12:12
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.

3 participants