chore(core): improve somke test

[NathanFlurry]

No description provided.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Skipped			Oct 18, 2025 0:15am

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

[claude]

Claude finished @NathanFlurry's task —— View job

---

Code Review for "improve somke test"

Todo List:

• Read and analyze changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

📝 Summary

This PR improves the smoke test by removing hardcoded localhost URLs and adding support for both WebSocket and HTTP connection methods. The changes enhance the test's flexibility and make it more suitable for different deployment environments.

🔍 Detailed Review

✅ Positive Changes

1. Environment Configuration Improvement (connect.ts:5, index.ts:74)

   • Removed hardcoded "http://localhost:6420" URLs
   • Now uses default client configuration, making tests more environment-agnostic
   • Better for CI/CD pipelines and different deployment contexts

2. Enhanced Test Coverage (spawn-actor.ts:33-65)

   • Added support for both WebSocket (ws) and HTTP connection methods
   • More comprehensive testing of different client interaction patterns
   • Good separation of concerns between connection types

⚠️ Issues & Concerns

Code Quality & Best Practices

1. Typo in Commit Message

   • Title contains "somke test" instead of "smoke test"
   • Consider fixing for better commit history

2. Hardcoded Method Selection (spawn-actor.ts:33)

   const method = "http" as "ws" | "http";

   • Method is hardcoded to "http", making the WebSocket branch unreachable
   • Consider making this configurable via environment variable or parameter
   • The WebSocket testing path will never execute in current state

3. Dead Code (spawn-actor.ts:36-53)

   • The entire WebSocket (ws) branch is unreachable due to hardcoded method
   • This reduces the actual test coverage despite adding the code

Potential Bugs

1. Resource Management Inconsistency

   • WebSocket path properly calls dispose() on connections
   • HTTP path doesn't call dispose(), which could lead to resource leaks
   • HTTP counters should also be properly disposed of

2. Error Handling

   • No validation that the selected method is valid
   • Could benefit from runtime validation of the method parameter

Performance Considerations

1. Sleep Duration (spawn-actor.ts:48, 61)

   • Both paths use 1000ms sleep, which seems reasonable for testing actor sleep behavior
   • No performance concerns here

2. Sequential Operations

   • HTTP path performs operations sequentially, which is appropriate for testing
   • Performance looks good overall

Security Concerns

1. No Security Issues Identified
   • Code doesn't handle sensitive data
   • No obvious security vulnerabilities

Test Coverage

1. Reduced Effective Coverage

   • Despite adding WebSocket test code, it's never executed
   • Current setup only tests HTTP path
   • Need to make method selection dynamic to achieve full coverage

2. Missing Disposal Testing

   • HTTP path doesn't test proper resource cleanup
   • Could miss resource leak issues

💡 Recommendations

1. Fix Method Selection

   const method = (process.env.CONNECTION_METHOD as "ws" | "http") ?? "http";

2. Add Resource Cleanup for HTTP

   else if (method === "http") {
       const key = ["test", testId, index.toString()];
       const counter = client.counter.getOrCreate(key);
       try {
           await counter.increment(1);
           await counter.increment(1);
           await new Promise((res) => setTimeout(res, 1000));
           await counter.increment(1);
       } finally {
           await counter.dispose?.(); // If dispose method exists
       }
   }

3. Add Method Validation

   if (!["ws", "http"].includes(method)) {
       throw new Error(`Invalid connection method: ${method}`);
   }

🎯 Overall Assessment

The changes move in the right direction by making the tests more environment-agnostic and adding infrastructure for testing multiple connection types. However, the hardcoded method selection prevents the WebSocket path from being tested and creates potential resource management issues in the HTTP path.

Recommendation: Address the method selection and resource cleanup issues before merging to maximize the value of these improvements.

---

[NathanFlurry]

• chore(core): improve somke test #1372 👈 (View in Graphite)
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• merge queue - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

[pkg-pr-new]

More templates

• example-ai-agent
• example-background-jobs
• example-better-auth-external-db
• example-bots
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-counter
• example-counter-serverless
• example-crdt
• example-database
• example-deno
• example-sqlite
• example-elysia
• example-express
• example-freestyle
• example-game
• example-hono
• example-hono-bun
• example-hono-react
• example-kitchen-sink
• example-next-js
• example-rate
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• example-smoke-test
• starter
• example-stream
• example-sync
• example-tenant
• example-trpc
• example-workflows

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/actor@1372

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/cloudflare-workers@1372

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/core@1372

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/db@1372

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/framework-base@1372

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/next-js@1372

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/react@1372

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivetkit@1372

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivetkit/@rivetkit/sql-loader@1372

commit: 45e1dd6

[claude]

Claude finished @NathanFlurry's task —— View job

---

Code Review for "improve somke test"

Todo List:

• Read and analyze changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

📝 Summary

This PR improves the smoke test by removing hardcoded localhost URLs and enhancing the connection testing logic. The changes make the test more flexible and realistic by supporting both WebSocket and HTTP connections with randomized selection.

🔍 Detailed Review

✅ Positive Changes

1. Environment Configuration Improvement (connect.ts:5, index.ts:74)

   • Removed hardcoded "http://localhost:6420" URLs
   • Now uses default client configuration, making tests more environment-agnostic
   • Better for CI/CD pipelines and different deployment contexts

2. Enhanced Randomized Testing (spawn-actor.ts:36)

   • Added randomized connection method selection between WebSocket and HTTP
   • More realistic testing pattern that exercises both connection types
   • Uses Math.random() > 0.5 for 50/50 distribution

3. Improved Resource Management (spawn-actor.ts:42)

   • WebSocket connections properly call dispose() to clean up resources
   • Good separation of concerns between connection types

⚠️ Issues & Concerns

Code Quality & Best Practices

1. Typo in PR Title

   • "somke test" should be "smoke test"
   • Consider fixing for better commit history

2. Inconsistent Resource Management (spawn-actor.ts:43-47)

   } else if (connMethod === "http") {
       const key = ["test", testId, index.toString()];
       const counter = client.counter.getOrCreate(key);
       await counter.increment(1);
   }

   • HTTP connections don't call dispose() while WebSocket connections do
   • This creates inconsistent resource cleanup patterns
   • Could lead to resource leaks in HTTP mode

3. Dead Code (spawn-actor.ts:66-67)

   if (succeeded) {
   }

   • Empty conditional block serves no purpose
   • Should be removed or have proper implementation

4. Console Logging in Production Code (spawn-actor.ts:54)

   console.log("sleeping", sleepTime);

   • Debug logging left in production code
   • Should use proper logging framework or be removed for production

Potential Bugs

1. Variable Naming Inconsistency (spawn-actor.ts:36)

   const connMethod = Math.random() > 0.5 ? "http" : "websocket";

   • Uses "websocket" but checks for "websocket" vs previous code used "ws"
   • This is actually correct, but ensures consistency

2. No Error Handling for Dispose (spawn-actor.ts:42)

   • dispose() call could potentially throw, but isn't wrapped in try-catch
   • Could mask the actual test error if dispose fails

Performance Considerations

1. Random Sleep Duration (spawn-test.ts:53)

   const sleepTime = 100 + Math.random() * 800; // 100-900ms range

   • Variable sleep time (100-900ms) adds realistic variability
   • Good for testing different timing scenarios
   • No performance concerns

2. Loop Structure (spawn-actor.ts:34)

   • 20 iterations per spawned actor is reasonable for stress testing
   • Performance impact is acceptable for smoke testing

Security Concerns

1. No Security Issues Identified
   • Code doesn't handle sensitive data
   • No obvious security vulnerabilities
   • Random values are used appropriately for testing

Test Coverage

1. Improved Coverage

   • Now tests both WebSocket and HTTP connection methods randomly
   • Better coverage of different connection scenarios
   • More realistic testing patterns

2. Resource Cleanup Testing

   • WebSocket path properly tests resource disposal
   • HTTP path doesn't test cleanup, missing coverage for resource management

💡 Recommendations

High Priority

1. Fix Resource Management Inconsistency

   } else if (connMethod === "http") {
       const key = ["test", testId, index.toString()];
       const counter = client.counter.getOrCreate(key);
       try {
           await counter.increment(1);
       } finally {
           // Add proper cleanup if HTTP counters support dispose
           if (counter.dispose) {
               await counter.dispose();
           }
       }
   }

2. Remove Dead Code

   // Remove this empty block
   if (succeeded) {
   }

Medium Priority

3. Improve Error Handling for Dispose

   try {
       await counter.dispose();
   } catch (disposeError) {
       // Log but don't fail the test for dispose errors
       console.warn('Failed to dispose counter:', disposeError);
   }

4. Replace Debug Logging

   // Remove or replace with proper logging
   // console.log("sleeping", sleepTime);

Low Priority

5. Add Validation for Connection Method

   const validMethods = ["http", "websocket"] as const;
   if (!validMethods.includes(connMethod)) {
       throw new Error(`Invalid connection method: ${connMethod}`);
   }

🎯 Overall Assessment

This is a solid improvement that enhances the smoke test's realism and flexibility. The randomized connection method selection is a good addition that will help catch issues in both connection types. The main concern is the inconsistent resource management between HTTP and WebSocket connections.

Recommendation: Address the resource management inconsistency and remove dead code before merging. The other improvements can be addressed in follow-up PRs.

Risk Level: Low - Changes improve test coverage and don't introduce breaking changes.

---

[claude]

Claude finished @NathanFlurry's task —— View job

---

Code Review for "improve somke test"

Todo List:

• Read and analyze changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

📝 Summary

This PR improves the smoke test by removing hardcoded localhost URLs and adding randomized connection method testing. The changes make the test more environment-agnostic and enhance test coverage by randomly selecting between WebSocket and HTTP connections.

🔍 Detailed Review

✅ Positive Changes

1. Environment Configuration Improvement (connect.ts:5, index.ts:74)

   • Removed hardcoded "http://localhost:6420" URLs from client creation
   • Now uses default client configuration, making tests more environment-agnostic
   • Better compatibility with CI/CD pipelines and different deployment contexts

2. Enhanced Test Coverage (spawn-actor.ts:36)

   • Added randomized connection method selection between WebSocket and HTTP
   • Uses Math.random() > 0.5 for 50/50 distribution between connection types
   • More realistic testing that exercises both connection paths

3. Performance Monitoring (spawn-actor.ts:37-50)

   • Proper performance timing measurement using performance.now()
   • Duration tracking for both connection methods
   • Good observability for test execution

⚠️ Issues & Concerns

Code Quality & Best Practices

1. Typo in PR Title

   • "somke test" should be "smoke test"
   • Consider fixing for better commit history readability

2. Inconsistent Resource Management (spawn-actor.ts:38-47)

   if (connMethod === "websocket") {
       // ... 
       await counter.dispose(); // ✅ Proper cleanup
   } else if (connMethod === "http") {
       // ... 
       // ❌ No dispose() call - potential resource leak
   }

   • WebSocket connections properly call dispose() for cleanup
   • HTTP connections don't call dispose(), creating inconsistent resource management
   • This could lead to resource leaks in HTTP mode

3. Dead Code (spawn-actor.ts:66-67)

   if (succeeded) {
   }

   • Empty conditional block serves no purpose
   • Should be removed or have proper implementation

4. Debug Logging in Production Code (spawn-actor.ts:54)

   console.log("sleeping", sleepTime);

   • Debug logging left in production code
   • Should use proper logging framework or be removed for production

5. Commented Code (spawn-actor.ts:55)

   // const sleepTime = 1000;

   • Leftover commented code should be removed

Potential Bugs

1. Resource Cleanup Inconsistency

   • Only WebSocket connections are properly disposed
   • HTTP connections may accumulate without proper cleanup
   • Could cause memory leaks in long-running tests

2. No Error Handling for Dispose (spawn-actor.ts:42)

   • dispose() call could potentially throw exceptions
   • Not wrapped in try-catch, could mask actual test errors

Performance Considerations

1. Random Sleep Duration (spawn-actor.ts:53)

   const sleepTime = 100 + Math.random() * 800; // 100-900ms range

   • Variable sleep time adds realistic variability to testing
   • Range of 100-900ms is reasonable for stress testing
   • No performance concerns

2. Loop Structure (spawn-actor.ts:34)

   • 20 iterations per spawned actor is appropriate for smoke testing
   • Performance impact is acceptable for test scenarios

Security Concerns

1. No Security Issues Identified
   • Code doesn't handle sensitive data or credentials
   • Random values used appropriately for testing purposes
   • No obvious security vulnerabilities

Test Coverage

1. Improved Coverage

   • Now tests both WebSocket and HTTP connection methods randomly
   • Better coverage of different connection scenarios than previous hardcoded approach
   • More realistic testing patterns

2. Resource Management Testing Gap

   • WebSocket path properly tests resource disposal
   • HTTP path doesn't test cleanup, missing coverage for resource management
   • Could miss resource leak issues in HTTP mode

💡 Recommendations

High Priority

1. Fix Resource Management Inconsistency

   } else if (connMethod === "http") {
       const key = ["test", testId, index.toString()];
       const counter = client.counter.getOrCreate(key);
       try {
           await counter.increment(1);
       } finally {
           // Add proper cleanup if HTTP counters support dispose
           if (counter.dispose) {
               await counter.dispose();
           }
       }
   }

2. Remove Dead Code

   // Remove this empty block entirely
   if (succeeded) {
   }

Medium Priority

3. Improve Error Handling for Dispose

   try {
       await counter.dispose();
   } catch (disposeError) {
       // Log but don't fail the test for dispose errors
       console.warn('Failed to dispose counter:', disposeError);
   }

4. Clean Up Debug Code

   // Remove or replace with proper logging
   // console.log("sleeping", sleepTime);
   // const sleepTime = 1000;

Low Priority

5. Add Connection Method Validation

   const validMethods = ["http", "websocket"] as const;
   if (!validMethods.includes(connMethod)) {
       throw new Error(`Invalid connection method: ${connMethod}`);
   }

🎯 Overall Assessment

This is a solid improvement that enhances the smoke test's flexibility and realism. The randomized connection method selection is valuable for catching issues in both connection types. The removal of hardcoded URLs makes the test more portable across environments.

Main Concerns: The inconsistent resource management between HTTP and WebSocket connections is the primary issue that should be addressed. The dead code and debug logging are minor cleanup items.

Recommendation: Address the resource management inconsistency before merging. The other improvements can be handled in follow-up PRs if needed.

Risk Level: Low - Changes improve test coverage without introducing breaking functionality.

---