Skip to content

framework test fixes - #5421

Merged
akshaydeo merged 1 commit into
devfrom
07-21-framework_test_fixes
Jul 21, 2026
Merged

akshaydeo merged 1 commit into
devfrom
07-21-framework_test_fixes

Conversation

@akshaydeo

@akshaydeo akshaydeo commented Jul 21, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Test packages (configstore, configstore/tables, and logstore) run in parallel against the same Postgres database, which previously caused race conditions where one package's setup/teardown would clobber another's tables. This PR isolates each test package into its own dedicated Postgres schema so they can run concurrently without interfering with each other.

Changes

  • Each test package now defines a pgTestSchema constant with a unique schema name (configstore_test, configstore_tables_test, logstore_test) and appends search_path=<schema> to its DSN so all DDL and DML is scoped to that schema automatically.
  • Each package's setup function now issues CREATE SCHEMA IF NOT EXISTS <schema> before running any migrations or table operations.
  • The deadlock test in configstore was updated to drop and recreate only its own schema instead of the shared public schema, preventing it from destroying tables owned by other test packages.
  • Cleanup logic and comments that previously worked around cross-package table sharing (e.g., leaving the migrations table intact for concurrent packages) were simplified now that each package has full ownership of its own schema.

Type of change

  • Bug fix
  • Feature
  • Refactor
  • Documentation
  • Chore/CI

Affected areas

  • Core (Go)
  • Transports (HTTP)
  • Providers/Integrations
  • Plugins
  • UI (React)
  • Docs

How to test

# Start the Postgres dependency
docker-compose -f framework/docker-compose.yml up -d postgres

# Run all affected test packages in parallel to confirm no cross-package interference
go test ./framework/configstore/... ./framework/logstore/... -v -count=1

Each package should complete without errors related to missing or unexpectedly modified tables.

Breaking changes

  • Yes
  • No

Related issues

Security considerations

No security implications. Changes are limited to test infrastructure.

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

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e158dd7-0c88-4079-81e8-a28b61e9ba3d

📥 Commits

Reviewing files that changed from the base of the PR and between 00d7a2f and 55213b4.

📒 Files selected for processing (4)
  • framework/configstore/migrations_test.go
  • framework/configstore/rdb_deadlock_postgres_test.go
  • framework/configstore/tables/encryption_test.go
  • framework/logstore/migrations_test.go

📝 Walkthrough

Summary by CodeRabbit

  • Tests
    • Improved PostgreSQL test isolation by running test data in dedicated schemas.
    • Prevented parallel test suites from interfering with shared database objects.
    • Ensured migration tests reliably start from a clean state.
    • Reduced cleanup impact on unrelated database schemas.

Walkthrough

Postgres-backed configstore and logstore tests now use dedicated schemas through search_path. Configstore setup and cleanup reset schema-local tables and migration state, while the deadlock test no longer modifies the shared public schema.

Changes

Postgres test isolation

Layer / File(s) Summary
Configstore schema setup and reset
framework/configstore/migrations_test.go, framework/configstore/rdb_deadlock_postgres_test.go
Configstore tests use a dedicated schema, reset config_providers within it, clear migration tracking, and avoid recreating public.
Encryption and logstore schema wiring
framework/configstore/tables/encryption_test.go, framework/logstore/migrations_test.go
Encryption and logstore test DSNs set search_path to dedicated schemas, which are created before migrations.

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

Suggested reviewers: pratham-mishra04, bearts

✨ 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 07-21-framework_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.

Copy link
Copy Markdown
Contributor Author

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

@akshaydeo
akshaydeo marked this pull request as ready for review July 21, 2026 14:54

Copy link
Copy Markdown
Contributor Author

Merge activity

  • Jul 21, 2:54 PM UTC: A user started a stack merge that includes this pull request via Graphite.

@akshaydeo
akshaydeo merged commit 7aaf3ba into dev Jul 21, 2026
13 of 15 checks passed
@akshaydeo
akshaydeo deleted the 07-21-framework_test_fixes branch July 21, 2026 14:54
@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

This looks safe to merge after making schema setup failures visible.

  • Package-specific schemas prevent the previous cross-package table collisions.
  • PostgreSQL resolves the configured search path correctly after each schema is created.
  • A schema permission error can silently remove PostgreSQL migration coverage from a test run.

framework/configstore/migrations_test.go

Important Files Changed

Filename Overview
framework/configstore/migrations_test.go Adds the configstore test schema and scopes migration setup and cleanup to it, but schema-creation failures are treated as an unavailable database.
framework/configstore/rdb_deadlock_postgres_test.go Replaces the shared public-schema reset with a reset of the configstore test schema.
framework/configstore/tables/encryption_test.go Moves PostgreSQL encryption tests into a dedicated configstore tables schema.
framework/logstore/migrations_test.go Moves logstore migration tests into a dedicated logstore schema.

Reviews (1): Last reviewed commit: "framework test fixes" | Re-trigger Greptile

Comment on lines 630 to +632

// All objects live in this package's dedicated schema (via search_path in
// the DSN), isolated from other test packages sharing the same database.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Schema Failure Silently Skips Coverage

When PostgreSQL is reachable but the test role cannot create configstore_test, this new branch returns nil, and callers treat that as an unavailable database and skip the PostgreSQL cases. CI can therefore pass without running the affected migration tests instead of reporting the schema-permission error.

akshaydeo added a commit that referenced this pull request Jul 21, 2026
* chore: fix migration tests

* framework test fixes (#5421)

* chore: adds docs for azure model router (#5175)

## Summary

Adds a dedicated documentation page for the Azure Model Router provider, explaining how Bifrost automatically falls back to Chat Completions when a model-router deployment is targeted via the Responses API.

## Changes

- Added `docs/providers/supported-providers/azure-model-router.mdx` documenting the Azure model-router routing behavior, supported operations, a Mermaid flowchart illustrating the fallback logic, usage examples (REST and Go SDK), and known limitations.
- Registered the new page in `docs/docs.json` so it appears in the navigation between the Azure and Bedrock entries.

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [x] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [x] Docs

## How to test

Navigate to the Azure Model Router page in the rendered docs and verify:
- The page appears in the sidebar between Azure and Bedrock.
- The Mermaid flowchart renders correctly.
- All code examples and notes display as expected.

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

## Security considerations

None. This is a documentation-only change with no impact on auth, secrets, or runtime behavior.

## Checklist

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

* fix: adds created timestamp for openai text completions (#5407)

## Summary

Adds the missing `created` field to `BifrostTextCompletionResponse` to align with the OpenAI text completion response schema, and modernizes the `ExtraParams` type alias from `map[string]interface{}` to the equivalent `map[string]any`.

## Changes

- Added `Created int` field with `omitempty` to `BifrostTextCompletionResponse`, representing the Unix timestamp (in seconds) of when the completion was created — this field was previously absent from the struct despite being part of the API response.
- Replaced `map[string]interface{}` with `map[string]any` in `TextCompletionParameters.ExtraParams` to use the modern Go type alias.

## Type of change

- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go version
go test ./...
```

Verify that text completion responses now include the `created` timestamp field when it is non-zero.

## Screenshots/Recordings

N/A

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

N/A

## Security considerations

None.

## Checklist

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

---------

Co-authored-by: Samyabrata Maji <116789799+sammaji@users.noreply.github.com>
Co-authored-by: Samyabrata Maji <samyabratamaji334@gmail.com>
@coderabbitai coderabbitai Bot mentioned this pull request Jul 21, 2026
akhsaul pushed a commit to akhsaul/bifrost that referenced this pull request Aug 27, 2026
akhsaul pushed a commit to akhsaul/bifrost that referenced this pull request Aug 27, 2026
* chore: fix migration tests

* framework test fixes (maximhq#5421)

* chore: adds docs for azure model router (maximhq#5175)

## Summary

Adds a dedicated documentation page for the Azure Model Router provider, explaining how Bifrost automatically falls back to Chat Completions when a model-router deployment is targeted via the Responses API.

## Changes

- Added `docs/providers/supported-providers/azure-model-router.mdx` documenting the Azure model-router routing behavior, supported operations, a Mermaid flowchart illustrating the fallback logic, usage examples (REST and Go SDK), and known limitations.
- Registered the new page in `docs/docs.json` so it appears in the navigation between the Azure and Bedrock entries.

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [x] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [x] Docs

## How to test

Navigate to the Azure Model Router page in the rendered docs and verify:
- The page appears in the sidebar between Azure and Bedrock.
- The Mermaid flowchart renders correctly.
- All code examples and notes display as expected.

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

## Security considerations

None. This is a documentation-only change with no impact on auth, secrets, or runtime behavior.

## Checklist

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

* fix: adds created timestamp for openai text completions (maximhq#5407)

## Summary

Adds the missing `created` field to `BifrostTextCompletionResponse` to align with the OpenAI text completion response schema, and modernizes the `ExtraParams` type alias from `map[string]interface{}` to the equivalent `map[string]any`.

## Changes

- Added `Created int` field with `omitempty` to `BifrostTextCompletionResponse`, representing the Unix timestamp (in seconds) of when the completion was created — this field was previously absent from the struct despite being part of the API response.
- Replaced `map[string]interface{}` with `map[string]any` in `TextCompletionParameters.ExtraParams` to use the modern Go type alias.

## Type of change

- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go version
go test ./...
```

Verify that text completion responses now include the `created` timestamp field when it is non-zero.

## Screenshots/Recordings

N/A

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

N/A

## Security considerations

None.

## Checklist

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

---------

Co-authored-by: Samyabrata Maji <116789799+sammaji@users.noreply.github.com>
Co-authored-by: Samyabrata Maji <samyabratamaji334@gmail.com>
occcat pushed a commit to occcat/bifrost that referenced this pull request Sep 2, 2026
occcat pushed a commit to occcat/bifrost that referenced this pull request Sep 2, 2026
* chore: fix migration tests

* framework test fixes (maximhq#5421)

* chore: adds docs for azure model router (maximhq#5175)

## Summary

Adds a dedicated documentation page for the Azure Model Router provider, explaining how Bifrost automatically falls back to Chat Completions when a model-router deployment is targeted via the Responses API.

## Changes

- Added `docs/providers/supported-providers/azure-model-router.mdx` documenting the Azure model-router routing behavior, supported operations, a Mermaid flowchart illustrating the fallback logic, usage examples (REST and Go SDK), and known limitations.
- Registered the new page in `docs/docs.json` so it appears in the navigation between the Azure and Bedrock entries.

## Type of change

- [ ] Bug fix
- [ ] Feature
- [ ] Refactor
- [x] Documentation
- [ ] Chore/CI

## Affected areas

- [ ] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [x] Docs

## How to test

Navigate to the Azure Model Router page in the rendered docs and verify:
- The page appears in the sidebar between Azure and Bedrock.
- The Mermaid flowchart renders correctly.
- All code examples and notes display as expected.

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

## Security considerations

None. This is a documentation-only change with no impact on auth, secrets, or runtime behavior.

## Checklist

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

* fix: adds created timestamp for openai text completions (maximhq#5407)

## Summary

Adds the missing `created` field to `BifrostTextCompletionResponse` to align with the OpenAI text completion response schema, and modernizes the `ExtraParams` type alias from `map[string]interface{}` to the equivalent `map[string]any`.

## Changes

- Added `Created int` field with `omitempty` to `BifrostTextCompletionResponse`, representing the Unix timestamp (in seconds) of when the completion was created — this field was previously absent from the struct despite being part of the API response.
- Replaced `map[string]interface{}` with `map[string]any` in `TextCompletionParameters.ExtraParams` to use the modern Go type alias.

## Type of change

- [ ] Bug fix
- [ ] Feature
- [x] Refactor
- [ ] Documentation
- [ ] Chore/CI

## Affected areas

- [x] Core (Go)
- [ ] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [ ] UI (React)
- [ ] Docs

## How to test

```sh
go version
go test ./...
```

Verify that text completion responses now include the `created` timestamp field when it is non-zero.

## Screenshots/Recordings

N/A

## Breaking changes

- [ ] Yes
- [x] No

## Related issues

N/A

## Security considerations

None.

## Checklist

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

---------

Co-authored-by: Samyabrata Maji <116789799+sammaji@users.noreply.github.com>
Co-authored-by: Samyabrata Maji <samyabratamaji334@gmail.com>
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