extend e2e API tests for DAC - #3552
Closed
akshaydeo wants to merge 3 commits into
Closed
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds deterministic e2e seeding (new e2eseed package + CLI) that seeds config and logs DBs, writes seed env and summary files, and extends API test tooling with a Postman collection merger and runner changes to inject seed env and expected manifests into Newman. ChangesE2E Seeding Infrastructure
API Test Collection Management
Sequence DiagramsequenceDiagram
participant CLI as e2eseed CLI
participant SeedBase
participant ConfigDB
participant LogsDB
participant EnvFile
CLI->>SeedBase: run(ctx, opts)
SeedBase->>ConfigDB: seedConfig(providers, governance, prompts, mcp)
SeedBase->>LogsDB: seedLogs(deterministic log rows, companions)
SeedBase->>EnvFile: WriteEnvFile(path, SeedEnv)
SeedBase-->>CLI: Summary (shapes, counts, paths)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
Confidence Score: 5/5Safe to merge — all changes are additive, the migration is idempotent and transactional, new columns are nullable so existing rows are unaffected, and the runner degrades gracefully when no extra collections are supplied. All changes are additive: new columns are nullable, indexes are built CONCURRENTLY, the runner falls back to the original collection path when --extra-collection is absent, and the seed command is a standalone CLI tool with no impact on the production request path. No files require special attention, though framework/e2eseed/seed.go has a deprecated strings.Title call that will surface in static analysis. Important Files Changed
Reviews (8): Last reviewed commit: "update mcp logs for dac" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
akshaydeo
force-pushed
the
05-17-extend_e2e_api_tests_for_dac
branch
from
May 17, 2026 15:48
ebe9dcb to
7904c7a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/cmd/e2eseed/main.go`:
- Around line 41-48: The DB handles returned by e2eseed.OpenDB (configDB and
logsDB) are opened in run but never closed; after each successful OpenDB call
add cleanup (e.g., defer configDB.Close() immediately after configDB is non-nil
and defer logsDB.Close() after logsDB is non-nil) or call the existing e2eseed
close helper if one exists, ensuring closures happen even on early returns;
reference the variables configDB and logsDB in main.run to place the defers or
close calls.
In `@framework/e2eseed/seed.go`:
- Around line 263-271: The loop over tables currently ignores the result of
Count by discarding Count(...).Error; capture the error from
configDB.WithContext(ctx).Table(table).Count(&count).Error and from
logsDB.WithContext(ctx).Table(table).Count(&count).Error and handle it instead
of swallowing it—e.g., if err != nil return the error (or wrap it with context
including table and the DB variable name); update the loops that reference
configDB, logsDB, ctx, table, count and ensure out[table] is only set after a
successful count so failures are propagated back to the caller.
- Around line 87-101: NormalizeOptions currently skips applying default DSNs so
opts.ConfigDSN and opts.LogsDSN can remain empty even though DefaultOptions
defines def.ConfigDSN and def.LogsDSN; update NormalizeOptions to set
opts.ConfigDSN = def.ConfigDSN and opts.LogsDSN = def.LogsDSN when those opts
fields are empty (same pattern used for ConfigDialect/LogsDialect/etc.),
referencing NormalizeOptions, DefaultOptions, opts.ConfigDSN, opts.LogsDSN,
def.ConfigDSN and def.LogsDSN to locate and apply the fix.
- Around line 466-468: The seeded Usage object sets TotalTokens inconsistently;
change the assignment so TotalTokens is computed from the other fields
(TotalTokens: PromptTokens + CompletionTokens) instead of a separate expression
(replace the current TotalTokens: 15 + index%50 with TotalTokens: (10 +
index%30) + (5 + index%20) or simply compute from PromptTokens and
CompletionTokens where the Usage struct is created) so TotalTokens always equals
PromptTokens + CompletionTokens (refer to the PromptTokens, CompletionTokens and
TotalTokens fields in the seed creation code).
In `@tests/e2e/api/README.md`:
- Around line 83-94: Update the API Management Extensions docs to mention the
environment-variable alternative: in the section that currently shows the CLI
flag --extra-collection, add a short paragraph showing how to set
BIFROST_API_EXTRA_COLLECTION as an environment variable and run
./runners/run-newman-api-tests.sh (reference the existing example using
--extra-collection and the runner script name run-newman-api-tests.sh so readers
can find the correct spot).
In `@tests/e2e/api/runners/merge-collections.mjs`:
- Around line 45-46: The code assumes base.item is an array by doing `base.item
= base.item || []` and then calling `base.item.push(...)`, which will throw if
base.item is a non-array truthy value; update the logic around `base.item` in
the merge routine to explicitly verify Array.isArray(base.item) before pushing,
e.g., if it's undefined set it to [], if it's a non-array throw a clear Error
indicating "base.item must be an array" (referencing the `base.item` variable
and the push site) so the test fails fast with an actionable message.
In `@tests/e2e/api/runners/run-newman-api-tests.sh`:
- Around line 100-110: Add explicit missing-value validation before each shift 2
for the flags that require arguments: --db-url, --logs-db-url, --config-path,
--extra-collection, --seed-env, and --expected; for each case branch (the case
labels matching these flag names) check that $2 is present and is not another
flag (e.g. non-empty and doesn't start with '-') and if the check fails print a
clear error to stderr and exit non-zero, otherwise assign the value to the
appropriate variable (DB_URL, LOGS_DB_URL, CONFIG_PATH, EXTRA_COLLECTIONS+=,
SEED_ENV_PATH, EXPECTED_PATH) and only then perform shift 2.
- Around line 325-328: The script currently sources SEED_ENV_PATH directly which
risks executing shell code; before the ". \"$SEED_ENV_PATH\"" call validate or
safely parse the file: check the file exists and then scan its lines (using
SEED_ENV_PATH) to only allow lines matching a strict KEY=VALUE pattern (e.g.,
/^[A-Za-z_][A-Za-z0-9_]*=.*$/) and reject lines containing metacharacters like
backticks, $(), ;&|<> or other suspicious syntax, or alternatively extract only
matching KEY=VALUE lines into a temporary cleaned file and source that; update
the run-newman-api-tests.sh section around set -a / . "$SEED_ENV_PATH" to
perform this validation/parsing and fail with a clear error if validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efb46bd7-419b-4aaf-b997-3194a1f934d1
📒 Files selected for processing (5)
framework/cmd/e2eseed/main.goframework/e2eseed/seed.gotests/e2e/api/README.mdtests/e2e/api/runners/merge-collections.mjstests/e2e/api/runners/run-newman-api-tests.sh
18 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/e2eseed/seed.go`:
- Around line 531-540: Replace the use of time.Now().UTC() with a fixed seed
timestamp variable (e.g., seedBaseTime) and use that single variable for all
seeded log timestamps so rows are deterministic; specifically update the
creation of mcp (mcp.Timestamp, Latency/Cost remain) and the async job
(CreatedAt and CompletedAt on logstore.AsyncJob) to use the fixed seedBaseTime
instead of now, and ensure any other nearby seeded records (the ones around the
job creation at the later block) also reference the same seedBaseTime.
- Around line 95-101: The current merge logic uses combined OR checks and can
overwrite an explicitly set dialect when only the DSN is missing; change the
code to fill each field individually and only when the resolved value is
non-empty. Specifically, replace the combined checks with per-field guards such
as: if opts.ConfigDialect == "" && resolved.ConfigDialect != "" {
opts.ConfigDialect = resolved.ConfigDialect } and if opts.ConfigDSN == "" &&
resolved.ConfigDSN != "" { opts.ConfigDSN = resolved.ConfigDSN }, and do the
same for opts.LogsDialect/opts.LogsDSN with
resolved.LogsDialect/resolved.LogsDSN so you never clobber an explicitly
provided value or assign empty defaults from resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9107a58-ee38-49e9-85e8-292bb0cf773e
📒 Files selected for processing (3)
framework/cmd/e2eseed/main.goframework/e2eseed/seed.gotests/e2e/api/runners/run-newman-api-tests.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/api/runners/run-newman-api-tests.sh
- framework/cmd/e2eseed/main.go
akshaydeo
force-pushed
the
05-04-dac_oss_visibility_checks
branch
from
May 18, 2026 02:44
833276a to
fc6f693
Compare
akshaydeo
force-pushed
the
05-17-extend_e2e_api_tests_for_dac
branch
from
May 18, 2026 02:44
f3deede to
9efb9c2
Compare
akshaydeo
force-pushed
the
05-17-extend_e2e_api_tests_for_dac
branch
from
May 18, 2026 03:34
9efb9c2 to
7ea6914
Compare
akshaydeo
force-pushed
the
05-17-extend_e2e_api_tests_for_dac
branch
from
May 18, 2026 05:24
7ea6914 to
07c68f5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
framework/e2eseed/seed.go (1)
72-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
seedBaseTimeis no longer deterministic.This package is the seed source for downstream e2e assertions, but anchoring it to
time.Now()makes every run produce different log timestamps and companion rows. That reintroduces the nondeterminism this stack was trying to remove.💡 Suggested fix
-var seedBaseTime = time.Now().UTC() +var seedBaseTime = time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/e2eseed/seed.go` around lines 72 - 76, seedBaseTime is set to time.Now(), making test seeds non-deterministic; change it to a deterministic value (for example a fixed UTC date via time.Date(...) or derived from a canonical constant) or make it configurable (read from an env var like SEED_BASE_TIME or accept an injected clock) so repeated runs produce identical timestamps; update the seedBaseTime variable initialization (symbol: seedBaseTime) and any docs/tests that rely on it to use the chosen deterministic source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/e2eseed/seed.go`:
- Around line 96-113: The code swallows errors from optionsFromConfigFile when
opts.ConfigPath is set, causing an explicit ConfigPath to silently fall back to
defaults; change the handling in the block that calls optionsFromConfigFile so
that if optionsFromConfigFile returns a non-nil error you return or log and exit
immediately (fail fast) instead of silently ignoring it; update the call-site
that reads opts.ConfigPath (the block that assigns opts.EncryptionKey,
opts.ConfigDialect, opts.ConfigDSN, opts.LogsDialect, opts.LogsDSN from
resolved) to check err != nil and propagate/return the error (or call log/exit)
and apply the same fix to the other identical occurrence that uses
optionsFromConfigFile later in seed.go.
- Around line 414-424: The seedConfig invocation still computes its own
timestamp and causes timestamps to drift on reruns; update the code paths that
call seedConfig (and any config-related seeding between the sections around
seedProviders/seedGovernance/seedPrompts/seedMCP and the later block at 430-515)
to reuse the same anchor time variable (now := time.Now().UTC()) passed into
seedConfig instead of calling time.Now() inside seedConfig, and ensure the
Assign(...).FirstOrCreate(...) calls use that passed-in now value so seeded rows
keep deterministic timestamps across reruns (look for the seedConfig function
and its use of Assign(...).FirstOrCreate(...) to apply the change).
---
Duplicate comments:
In `@framework/e2eseed/seed.go`:
- Around line 72-76: seedBaseTime is set to time.Now(), making test seeds
non-deterministic; change it to a deterministic value (for example a fixed UTC
date via time.Date(...) or derived from a canonical constant) or make it
configurable (read from an env var like SEED_BASE_TIME or accept an injected
clock) so repeated runs produce identical timestamps; update the seedBaseTime
variable initialization (symbol: seedBaseTime) and any docs/tests that rely on
it to use the chosen deterministic source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e777701c-b773-4cae-85e7-b8472abeb497
📒 Files selected for processing (6)
core/schemas/bifrost.goframework/cmd/e2eseed/main.goframework/e2eseed/seed.gotests/e2e/api/README.mdtests/e2e/api/runners/merge-collections.mjstests/e2e/api/runners/run-newman-api-tests.sh
✅ Files skipped from review due to trivial changes (1)
- core/schemas/bifrost.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/api/README.md
- tests/e2e/api/runners/merge-collections.mjs
- tests/e2e/api/runners/run-newman-api-tests.sh
- framework/cmd/e2eseed/main.go
akshaydeo
force-pushed
the
05-17-extend_e2e_api_tests_for_dac
branch
from
May 18, 2026 05:57
07c68f5 to
33867ee
Compare
akshaydeo
force-pushed
the
05-17-extend_e2e_api_tests_for_dac
branch
from
May 18, 2026 06:10
6976390 to
28bf018
Compare
akshaydeo
force-pushed
the
05-04-dac_oss_visibility_checks
branch
from
May 18, 2026 06:10
fc6f693 to
0bae4bd
Compare
… and fixes hot-reload of plugin
akshaydeo
force-pushed
the
05-04-dac_oss_visibility_checks
branch
from
May 18, 2026 06:13
0bae4bd to
686203b
Compare
akshaydeo
force-pushed
the
05-17-extend_e2e_api_tests_for_dac
branch
from
May 18, 2026 06:13
28bf018 to
036bea6
Compare
akshaydeo
changed the base branch from
05-04-dac_oss_visibility_checks
to
graphite-base/3552
May 18, 2026 06:14
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds support for merging enterprise-only Postman collections into the API management e2e test runner without committing them to OSS. This allows enterprise repos to supply additional API test coverage (e.g. DAC-specific assertions) that runs alongside the shared OSS management requests.
Changes
merge-collections.mjs, a Node.js script that merges one or more extension Postman collections into a base collection, wrapping each extension's items in a named folder and writing the result to a temporary output file.--extra-collection <path>flag torun-newman-api-tests.sh, which can be specified multiple times. When present, the runner invokesmerge-collections.mjsto produce a merged collection before passing it to Newman.BIFROST_API_EXTRA_COLLECTIONenvironment variable as an alternative to the CLI flag.--extra-collectionflag and the intended OSS/enterprise split.Type of change
Affected areas
How to test
Run the API management tests with an extra enterprise collection:
cd tests/e2e/api ./runners/run-newman-api-tests.sh --extra-collection /path/to/enterprise.postman_collection.jsonVerify that the merged collection file is written to the report directory as
api-management-with-extensions.postman_collection.jsonand that Newman runs against it.Run without
--extra-collectionto confirm the default OSS behavior is unchanged.The
BIFROST_API_EXTRA_COLLECTIONenvironment variable can also be used in place of the flag:Breaking changes
Related issues
Security considerations
Extra collection paths are validated to exist on disk before merging. No secrets or credentials are introduced; the feature only affects which Postman items are included in a test run.
Checklist
docs/contributing/README.mdand followed the guidelines