refactor(engine): extract test runner to standalone rivet-engine-runner package#4089
refactor(engine): extract test runner to standalone rivet-engine-runner package#4089NathanFlurry wants to merge 1 commit intomainfrom
Conversation
…er package - Move runner code from engine/packages/engine/tests/common/test_runner/ to engine/sdks/rust/engine-runner/ - Add RunnerConfig builder pattern matching TypeScript engine-runner API - Update test wrapper to use the new package via re-exports - Add workspace dependency for rivet-engine-runner - Enables reuse of runner logic outside of tests (e.g., openworkers integration)
|
🚅 Deployed to the rivet-pr-4089 environment in rivet-frontend
|
How to use the Graphite Merge QueueAdd the label merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Pull Request Review: Extract Test Runner to Standalone PackageSummaryThis PR successfully refactors the test runner from ✅ Strengths
🔍 Issues FoundCritical Issues1. Incorrect Example in Documentation (engine/sdks/rust/engine-runner/src/lib.rs:10-25) The example code will not compile: let mut runner = Runner::new(config)?; // ❌ Runner::new() doesn't exist
runner.register_actor("echo", |_| Box::new(EchoActor::new())); // ❌ No register_actor methodCorrect usage should be: let config = RunnerConfig::builder()
.endpoint("http://127.0.0.1:8080")
.token("dev")
.namespace("my-namespace")
.runner_name("my-runner")
.runner_key("unique-key")
.build()?;
let runner = RunnerBuilder::new(config)
.with_actor_behavior("echo", |_| Box::new(EchoActor::new()))
.build()?;
runner.start().await?;2. Unused The
Either use it or remove it from the public API. Minor Issues3. Dead Code Warnings (engine/sdks/rust/engine-runner/src/runner.rs:175-178) struct ActorState {
#[allow(dead_code)]
actor_id: String,
#[allow(dead_code)]
generation: u32,
actor: Box<dyn TestActor>,
}These fields should either be used (e.g., for debug logging) or the struct should be simplified. The 4. Missing Default Implementations (engine/sdks/rust/engine-runner/src/behaviors.rs) Good additions of 5. Removed Helper Function (engine/packages/engine/tests/common/test_helpers.rs:82-103) The
6. Type Inconsistency (engine/sdks/rust/engine-runner/src/runner.rs:889) pub async fn get_actor_ids(&self) -> Vec<String> {
let actors = self.actors.lock().await;
actors.keys().cloned().collect()
}Previously returned 🔒 Security ConsiderationsNo security issues identified. The refactoring:
⚡ Performance ConsiderationsNo performance regressions expected:
🧪 Test CoverageMissing: No unit tests for the new standalone package itself. The existing tests in
📝 Code Quality & StyleAdherence to CLAUDE.md Guidelines✅ Good:
🎯 RecommendationsMust Fix
Should Fix
Nice to Have
📊 Overall AssessmentQuality: ⭐⭐⭐⭐ (4/5) This is a solid refactoring that successfully extracts test infrastructure into a reusable package. The code is well-structured and maintains backward compatibility. The main issues are documentation accuracy and some unused fields. After addressing the critical issues (documentation example and metadata field), this will be production-ready. Recommendation: Approve with requested changes to documentation example and metadata field handling. |
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: