Skip to content

fix: test fixes - #4823

Merged
Pratham-Mishra04 merged 1 commit into
devfrom
07-01-fix_test_fixes
Jul 2, 2026
Merged

fix: test fixes#4823
Pratham-Mishra04 merged 1 commit into
devfrom
07-01-fix_test_fixes

Conversation

@roroghost17

@roroghost17 roroghost17 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the from_memory flag behavior for the virtual keys endpoint so it correctly reads from in-memory GovernanceData instead of falling through to the DB-backed ConfigStore. Also corrects the casing of an invalid request payload error message.

Changes

  • The from_memory query parameter now routes virtual key lookups through GetGovernanceData (in-memory path) and skips all ConfigStore calls, including the paginated variant. Previously, the flag was incorrectly routing to the ConfigStore regardless.
  • mockGovernanceManagerForVK was updated to hold and return a configurable GovernanceData value instead of always returning nil, enabling accurate test assertions.
  • TestGetVirtualKeys_FromMemoryUsesConfigStore was renamed to TestGetVirtualKeys_FromMemoryUsesGovernanceData and TestGetVirtualKeys_FromMemoryWithLimitUsesPaginatedConfigStore was renamed to TestGetVirtualKeys_FromMemoryTakesPrecedenceOverLimit to reflect the corrected behavior. Assertions were inverted accordingly: GetGovernanceData is now expected to be called once, and ConfigStore methods are expected to receive zero calls.
  • The invalid request payload error message was corrected from "invalid request payload" to "Invalid request payload" to match the actual returned string.

Type of change

  • Bug fix

Affected areas

  • Transports (HTTP)

How to test

go test ./transports/bifrost-http/handlers/...

Verify that TestGetVirtualKeys_FromMemoryUsesGovernanceData, TestGetVirtualKeys_FromMemoryTakesPrecedenceOverLimit, and TestPrepareRequestInvalidPayloadDoesNotExposeDecoderDetails all pass.

Breaking changes

  • Yes
  • No

Security considerations

No security implications. The change ensures in-memory governance data is served when requested rather than hitting the database, which is the intended isolation behavior.

Checklist

  • I read docs/contributing/README.md and followed the guidelines
  • I added/updated tests where appropriate
  • I updated documentation where needed
  • I verified builds succeed (Go and UI)
  • I verified the CI pipeline passes locally if applicable

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

An error occurred during the review process. Please try again later.

📝 Walkthrough

Walkthrough

This PR updates test files only. Governance tests are revised so getVirtualKeys with from_memory=true uses in-memory GovernanceData via GetGovernanceData instead of ConfigStore-backed queries. A separate test adjusts an expected error message capitalization.

Changes

Test Assertion Updates

Layer / File(s) Summary
Governance from_memory hydration tests
transports/bifrost-http/handlers/governance_test.go
Mock manager now returns a GovernanceData payload from GetGovernanceData; tests are rewritten to assert from_memory=true calls GetGovernanceData exactly once and skips ConfigStore's GetVirtualKeys/GetVirtualKeysPaginated calls, including with pagination/limit params.
Request payload error message assertion
transports/bifrost-http/handlers/requestpayload_test.go
Expected error string updated from lowercase to capitalized "Invalid request payload".

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • maximhq/bifrost#3903: Both PRs modify the same governance test file's from_memory behavior for getVirtualKeys, asserting opposite call-path expectations.

Suggested reviewers: akshaydeo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is too generic and does not clearly describe the actual change. Use a concise title that names the main fix, such as the from_memory virtual-keys behavior correction.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description matches the template well and covers summary, changes, testing, type, affected area, breaking changes, and security.
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 unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 07-01-fix_test_fixes

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


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

@CLAassistant

CLAassistant commented Jul 1, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@roroghost17
roroghost17 marked this pull request as ready for review July 1, 2026 11:53
@coderabbitai
coderabbitai Bot requested a review from akshaydeo July 1, 2026 11:54
@greptile-apps

greptile-apps Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

Test-only changes that align assertions with the handler's existing behavior; no production code modified.

Both files are test-only. The governance test mock and assertion changes precisely match what the handler already does (early-return from the from_memory branch without touching ConfigStore). The requestpayload fix corrects a string literal that mismatched the live error message.

No files require special attention.

Important Files Changed

Filename Overview
transports/bifrost-http/handlers/governance_test.go Test assertions corrected to match actual handler behavior: GetGovernanceData is now expected once and ConfigStore calls are expected to be zero for the from_memory path.
transports/bifrost-http/handlers/requestpayload_test.go Single-character casing fix — expected error string updated from 'invalid request payload' to 'Invalid request payload' to match the actual fmt.Errorf string in inference.go.

Reviews (2): Last reviewed commit: "fix: test fixes" | Re-trigger Greptile

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 1, 2026
@akshaydeo
akshaydeo dismissed coderabbitai[bot]’s stale review July 1, 2026 12:24

The merge-base changed after approval.

@akshaydeo
akshaydeo requested a review from a team as a code owner July 1, 2026 12:24
@roroghost17
roroghost17 force-pushed the 07-01-fix_test_fixes branch from 5b9b1e2 to 27423a7 Compare July 1, 2026 12:51

Pratham-Mishra04 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Merge activity

  • Jul 2, 7:47 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jul 2, 7:48 AM UTC: @Pratham-Mishra04 merged this pull request with Graphite.

@Pratham-Mishra04
Pratham-Mishra04 merged commit 98645f7 into dev Jul 2, 2026
12 of 16 checks passed
@Pratham-Mishra04
Pratham-Mishra04 deleted the 07-01-fix_test_fixes branch July 2, 2026 07:48
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