chore: fix the BenchmarkExecutionEngine#1288
Conversation
Also fix the corresponding test for static datasource.
ysmolski
requested review from
a team,
Noroth,
StarpTech,
devsergiy and
jensneuse
as code owners
WalkthroughAdds benchmark invocation to test-quick targets in Makefiles and updates tests to use JSON-shaped static data and adjusted mappings; tests reflect removal of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
execution/Makefile (1)
9-9: Use -run=^$ to skip tests reliably (and consider -benchmem).
-run=xxxxcould accidentally match a test name.-run=^$guarantees no tests run while benchmarks do. Adding-benchmemis often helpful and low-cost.- go test -run=xxxx -bench=. -benchtime=1x ./... + go test -run=^$ -bench=. -benchmem -benchtime=1x ./...execution/engine/execution_engine_test.go (1)
4724-4729: Avoid silently ignoring Execute errors in the hot loop.A cheap check won't skew results and prevents false-green runs if something regresses.
- _ = bc.engine.Execute(ctx, &req, bc.writer) + if err := bc.engine.Execute(ctx, &req, bc.writer); err != nil { + b.Fatal(err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
execution/Makefile(1 hunks)execution/engine/execution_engine_test.go(2 hunks)v2/pkg/engine/datasource/staticdatasource/static_datasource_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
v2/pkg/engine/datasource/staticdatasource/static_datasource_test.go (1)
pkg/introspection/introspection.go (1)
TypeName(42-44)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (6)
execution/engine/execution_engine_test.go (2)
4671-4671: LGTM: static datasource now serves JSON-shaped payload.Switching
Datato{"hello": "world"}aligns the source with the requested field shape.
4682-4683: LGTM: trimmed FieldConfiguration matches new defaults.Using only
TypeNameandFieldNameis consistent with the removal ofDisableDefaultMapping.v2/pkg/engine/datasource/staticdatasource/static_datasource_test.go (4)
27-27: LGTM: static fetch input updated to JSON object.Matches the field shape and static source semantics.
38-39: LGTM: explicit Path on resolve.String.Reflects the new
Pathfield inresolve.Stringand clarifies mapping.
61-61: LGTM: plan configuration data matches fetch input.Consistent JSON object payload.
70-72: LGTM: minimal FieldConfiguration.Removal of
DisableDefaultMappingis correctly reflected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/Makefile (1)
14-14: Correctly skip tests and run only benchmarks; consider adding -benchmem.Escaping with ^$$ is right for Make to pass ^$ to go test. Optional: include -benchmem for alloc stats.
- go test -run=^$$ -bench=. -benchtime=1x ./... + go test -run=^$$ -bench=. -benchtime=1x -benchmem ./...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
execution/Makefile(1 hunks)execution/engine/execution_engine_test.go(3 hunks)v2/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/Makefile
🧰 Additional context used
🧬 Code graph analysis (1)
execution/engine/execution_engine_test.go (2)
v2/pkg/introspection/introspection.go (1)
Data(10-12)pkg/introspection/introspection.go (1)
TypeName(42-44)
⏰ 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). (4)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (3)
execution/engine/execution_engine_test.go (3)
4671-4671: Static datasource payload now matches field shape.Switching Data to a JSON object aligns with default mapping for Query.hello.
4682-4683: FieldConfiguration trimmed to TypeName/FieldName is correct post-API change.No DisableDefaultMapping needed; default mapping will resolve hello.
4727-4729: Benchmark now fails fast on Execute error.Good addition; avoids false-positive perf numbers when execution fails.
devsergiy
deleted the
yury/eng-7627-engine-fix-executionbenchmarkexecutionengine
branch
2 participants
Also fix the corresponding test for static datasource.
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Chores
Checklist