Merged
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
rustatian
added
the
C-enhancement
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces comprehensive end-to-end testing infrastructure for RoadRunner, including a GitHub Actions workflow, test harness, mock logger, RPC helpers, Go test suites covering gRPC, HTTP, and Jobs plugins, PHP worker implementations, and configurations with optional OpenTelemetry support. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test
participant E as Endure DI<br/>Container
participant P as Plugins<br/>(gRPC, RPC, Server)
participant W as PHP Worker<br/>(EchoService)
participant C as gRPC Client
T->>E: Init plugins + config
T->>E: Start container (Serve)
E->>P: Initialize & wire dependencies
E->>P: Serve (start all plugins)
P->>W: Spawn PHP worker process
W->>W: Register EchoService
T->>C: Dial gRPC (127.0.0.1:9191)
C->>W: Send Ping("hello")
W->>W: Convert to "HELLO"
W->>C: Return Message{Msg:"HELLO"}
C->>T: Assert response matches
T->>P: Trigger stop via signal
P->>W: Terminate worker
E->>E: Stop all plugins
T->>T: Cleanup
sequenceDiagram
participant T as Test
participant E as Endure DI<br/>Container
participant HP as HTTP Plugin
participant MW as Middleware<br/>(headers, gzip, etc)
participant W as PHP Worker<br/>(echo handler)
participant HT as HTTP Client
T->>E: Init plugins + config
T->>E: Start container
E->>HP: Initialize HTTP on :18950
E->>MW: Attach middleware stack
E->>W: Spawn PHP worker
HT->>HP: GET request
HP->>MW: Process through stack
MW->>W: Forward to worker
W->>W: Execute handler (uppercase)
W->>MW: Return response + body
MW->>HT: Apply gzip, headers
HT->>T: Assert 201, header, decompressed body
T->>E: Stop via signal
E->>E: Cleanup
sequenceDiagram
participant T as Test
participant E as Endure DI<br/>Container
participant JO as Jobs Plugin
participant MP as Memory Pipeline
participant W as PHP Worker<br/>(Consumer)
T->>E: Init plugins + config
T->>E: Start container
E->>JO: Initialize jobs
E->>MP: Create memory pipelines
E->>W: Spawn PHP worker
T->>JO: Push job to test-1 pipeline
JO->>MP: Queue job (memory)
MP->>W: Dispatch job
W->>W: Consume & ack
W->>JO: Acknowledge completion
T->>JO: Verify job processed
T->>JO: Destroy pipelines
JO->>MP: Drain pipelines
T->>E: Stop container
E->>E: Cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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. 🔧 OpenGrep (1.16.4)tests/e2e_grpc_test.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m tests/e2e_jobs_test.go┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a new end-to-end (e2e) test suite that boots a RoadRunner container via Endure, spawns real PHP workers, and validates behavior across HTTP middleware/static files, Jobs, and gRPC. It also introduces supporting test fixtures (PHP scripts, protobuf stubs, configs), a zap-based mock logger plugin for assertions, and a GitHub Actions workflow to run these e2e tests in CI.
Changes:
- Add Go-based e2e tests for Jobs (memory driver), HTTP middleware/static serving, and gRPC echo.
- Add PHP worker fixtures + protobuf definitions/generated stubs to back the e2e scenarios.
- Add CI workflow to run the e2e suite with Go + PHP, including Composer and Go module caching.
Reviewed changes
Copilot reviewed 36 out of 40 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testdata/sample.txt | Static file fixture used by the HTTP static middleware e2e test. |
| tests/proto/service/service.proto | Proto definition for the gRPC Echo service used in e2e. |
| tests/proto/service/service.pb.go | Generated Go protobuf types for the Echo service messages. |
| tests/proto/service/service_grpc.pb.go | Generated Go gRPC client/server interfaces for Echo. |
| tests/php_test_files/jobs/jobs_ok.php | PHP jobs consumer fixture that immediately ACKs tasks. |
| tests/php_test_files/http/echo.php | PHP HTTP handler fixture (uppercases query param, returns 201). |
| tests/php_test_files/http/client.php | Generic PHP PSR-7 worker dispatcher for e2e HTTP tests. |
| tests/php_test_files/grpc/worker-grpc.php | PHP gRPC worker entrypoint for e2e gRPC test. |
| tests/php_test_files/grpc/src/Service/Message.php | Generated PHP protobuf message type for Echo. |
| tests/php_test_files/grpc/src/Service/EchoInterface.php | Generated PHP GRPC service interface for Echo. |
| tests/php_test_files/grpc/src/HealthService.php | PHP implementation of gRPC health service for tests. |
| tests/php_test_files/grpc/src/Health/HealthInterface.php | Generated PHP interface for gRPC health service. |
| tests/php_test_files/grpc/src/Health/HealthCheckResponse/ServingStatus.php | Generated PHP enum for health serving status. |
| tests/php_test_files/grpc/src/Health/HealthCheckResponse.php | Generated PHP protobuf message for health response. |
| tests/php_test_files/grpc/src/Health/HealthCheckRequest.php | Generated PHP protobuf message for health request. |
| tests/php_test_files/grpc/src/GPBMetadata/Service.php | Generated PHP descriptor metadata for service.proto. |
| tests/php_test_files/grpc/src/EchoService.php | PHP implementation of Echo service (uppercases msg). |
| tests/php_test_files/composer.json | PHP dependencies for the e2e fixtures (RR worker libs, PSR-7, etc.). |
| tests/php_test_files/.gitignore | Ignores vendor/ and composer.lock inside PHP test fixtures. |
| tests/mock/observer.go | In-memory zap observer implementation for capturing logs in tests. |
| tests/mock/logger.go | Endure plugin providing a zap logger wired to the observer. |
| tests/mock/doc.go | Package doc for the mock logger plugin. |
| tests/helpers/helpers.go | Helper functions for Goridge RPC calls for jobs lifecycle tests. |
| tests/helpers/doc.go | Package doc for test helpers. |
| tests/go.sum | Go dependency lock for the tests module. |
| tests/go.mod | Standalone Go module for e2e tests and their dependencies. |
| tests/e2e_jobs_test.go | E2E test for Jobs lifecycle using the memory driver. |
| tests/e2e_http_test.go | E2E tests for HTTP middleware stack + static file serving. |
| tests/e2e_grpc_test.go | E2E test for gRPC Echo Ping roundtrip through PHP worker. |
| tests/doc.go | Package doc describing the purpose of the e2e tests package. |
| tests/configs/.rr-jobs-memory.yaml | RoadRunner config for Jobs (memory driver) e2e test. |
| tests/configs/.rr-http-static.yaml | RoadRunner config for HTTP static middleware e2e test. |
| tests/configs/.rr-http-middleware.yaml | RoadRunner config for HTTP middleware chain e2e test. |
| tests/configs/.rr-grpc.yaml | RoadRunner config for gRPC e2e test including proto registration. |
| go.work.sum | Workspace dependency sum updates. |
| go.work | Adds ./tests to the Go workspace. |
| go.sum | Root module sum updates (dependency bumps). |
| go.mod | Root module dependency bumps (context/genproto updates). |
| .gitignore | Ignore vendor/ and composer.lock globally. |
| .github/workflows/e2e.yml | New GitHub Actions workflow to run e2e tests with Go+PHP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 7
🧹 Nitpick comments (7)
tests/configs/.rr-jobs-memory.yaml (1)
14-19: Add asupervisorblock to keep pool config consistent.The pool settings include workers/jobs/timeouts but skip
supervisor, which diverges from the established.rr*.yamlpool pattern used across plugins.Based on learnings: "Applies to .rr*.yaml : Use consistent pool configuration pattern across plugins with parameters: num_workers, max_jobs, timeouts, and supervisor".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/configs/.rr-jobs-memory.yaml` around lines 14 - 19, The pool block is missing the standard supervisor subsection; update the tests/configs/.rr-jobs-memory.yaml by adding a supervisor block under the existing pool configuration so it matches the .rr*.yaml pattern (keep pool: num_workers, max_jobs, allocate_timeout, destroy_timeout and add supervisor with the same keys used across plugins). Locate the pool block (containing num_workers, max_jobs, allocate_timeout, destroy_timeout) and insert a supervisor subsection consistent with other .rr*.yaml files so the config pattern is uniform.tests/proto/service/service.proto (1)
3-4: Buf lint warning: package directory mismatch.The static analyzer reports that files with package
serviceshould be in a directory namedservicerelative to the Buf root, but this file is intests/proto/service. This is a common lint warning when the proto path structure doesn't match the package name exactly.For test purposes, this is acceptable if Buf is not enforced in CI. If you want to silence this warning, you can either:
- Rename the package to match the directory structure (e.g.,
package tests.proto.service;)- Configure Buf to exclude test protos from this lint rule
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/proto/service/service.proto` around lines 3 - 4, The proto file declares package "service" which doesn't match its directory layout, triggering Buf's package-directory lint; either update the package declaration to reflect the directory (e.g., change the package line from "package service;" to a namespaced package like "package tests.proto.service;") and adjust option go_package accordingly, or instead modify your Buf configuration to exclude test protos from this lint rule; locate and edit the package declaration and the option go_package in the service.proto (symbols: the "package" statement and the "option go_package" setting) to keep them consistent with whichever approach you choose.tests/php_test_files/grpc/src/HealthService.php (1)
13-24: Optional DRY cleanup for duplicated response construction.Line 13 and Line 20 currently duplicate the same response-building logic. Consider extracting a helper to keep both RPC handlers lean.
♻️ Suggested cleanup
class HealthService implements HealthInterface { public function Check(ContextInterface $ctx, HealthCheckRequest $in): HealthCheckResponse { - $out = new HealthCheckResponse(); - $out->setStatus(HealthCheckResponse\ServingStatus::SERVING); - return $out; + return $this->servingResponse(); } public function Watch(ContextInterface $ctx, HealthCheckRequest $in): HealthCheckResponse { - $out = new HealthCheckResponse(); - $out->setStatus(HealthCheckResponse\ServingStatus::SERVING); - return $out; + return $this->servingResponse(); } + + private function servingResponse(): HealthCheckResponse + { + $out = new HealthCheckResponse(); + return $out->setStatus(HealthCheckResponse\ServingStatus::SERVING); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/php_test_files/grpc/src/HealthService.php` around lines 13 - 24, Both Check and Watch in HealthService duplicate the HealthCheckResponse construction; extract a private helper (e.g., buildServingResponse or createHealthResponse) that returns a HealthCheckResponse with status set to Serving, and call that helper from both public methods (Check and Watch) to remove the duplicated response-building logic.tests/e2e_grpc_test.go (1)
91-91: Replace fixed startup sleep with an active readiness check.
time.Sleep(time.Second)is timing-sensitive and can flake on slower CI nodes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e_grpc_test.go` at line 91, Replace the flaky fixed sleep (time.Sleep(time.Second)) with an active readiness poll: instead of sleeping, loop with a short backoff up to a timeout and attempt a real readiness check (e.g., grpc.Dial + client health call or a simple RPC/health endpoint probe) until it succeeds; use context with deadline and return/fail the test on timeout. Locate the usage of time.Sleep in the e2e test (tests/e2e_grpc_test.go) and replace it with a helper that repeatedly tries the gRPC connection/health check and only proceeds when the server is ready.tests/mock/logger.go (1)
71-72: Optionally preserve logger names inNamedLogger.Returning
l.base.Named(name)keeps component labeling behavior closer to production.Suggested fix
-func (l *Log) NamedLogger(string) *zap.Logger { - return l.base +func (l *Log) NamedLogger(name string) *zap.Logger { + return l.base.Named(name) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mock/logger.go` around lines 71 - 72, The NamedLogger method should forward the provided name to the underlying zap logger instead of always returning l.base; update the function to use the parameter (e.g., change the signature to func (l *Log) NamedLogger(name string) *zap.Logger) and return l.base.Named(name) so component labels are preserved (ensure the parameter name matches usage).tests/mock/observer.go (2)
186-193: Consider using named fields in struct literal.Using named fields improves readability and guards against field reordering.
🔧 Suggested improvement
func (co *contextObserver) Write(ent zapcore.Entry, fields []zapcore.Field) error { all := make([]zapcore.Field, 0, len(fields)+len(co.context)) all = append(all, co.context...) all = append(all, fields...) - co.logs.add(LoggedEntry{ent, all}) + co.logs.add(LoggedEntry{Entry: ent, Context: all}) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mock/observer.go` around lines 186 - 193, In contextObserver.Write, replace the positional struct literal LoggedEntry{ent, all} with a named-field literal (e.g., LoggedEntry{Entry: ent, Fields: all}) so the creation of LoggedEntry is clear and robust against field reordering; update the LoggedEntry construction in the co.logs.add call to use the struct's field names.
1-29: Code adapted from zap's observer package.This appears to be adapted from
go.uber.org/zap/zaptest/observer. If there's no specific customization needed, consider importing the existing package directly (go.uber.org/zap/zaptest/observer) to reduce maintenance burden and benefit from upstream improvements.If custom behavior is required, consider documenting the reason for vendoring this code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mock/observer.go` around lines 1 - 29, This file (package mocklogger) is a near-copy of zap's observer implementation; either remove the vendored code and import the upstream package go.uber.org/zap/zaptest/observer where tests use the observer types, or if you must keep a customized copy, add a top-of-file comment explaining what was changed and why (documenting differences and maintenance responsibility) and ensure exported types/functions match the usages in tests so behavior is explicit; reference the package name mocklogger and any observer-related types in your tests when making the replacement or documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 28: The .gitignore entry "**/composer.lock" causes composer.lock to be
ignored (allowing PHP test deps to drift); remove or override that ignore so e2e
fixture lockfiles are committed: either delete the "**/composer.lock" pattern or
add a negate rule to track composer.lock for test fixtures (ensure the pattern
"**/composer.lock" is no longer preventing committing fixture lockfiles) and
then commit the composer.lock for deterministic CI runs.
In `@tests/e2e_grpc_test.go`:
- Around line 13-14: Replace the local relative imports with the
module-qualified paths so the test imports resolve correctly: change the import
of mocklogger (currently "tests/mock") to
"github.com/roadrunner-server/roadrunner/v2025/tests/mock" and change the
service proto import (currently "tests/proto/service") to
"github.com/roadrunner-server/roadrunner/v2025/tests/proto/service"; update the
import block where mocklogger and service are referenced and run go fmt/go mod
tidy to ensure module resolution.
- Around line 66-89: The background goroutine started in wg.Go should not call
assert.Fail/FailNow or write to the shared err variable; instead, capture errors
and stop results by sending them over a dedicated error channel (e.g., errCh) or
closing a done channel so the main test goroutine can perform assertions and
call t.Fatal/t.FailNow; update the worker loop that reads from ch, sig, and
stopCh to send any encountered error from cont.Stop() or e.Error to errCh (or a
struct with context) and return, and change the main test flow to read from
errCh and perform assert/FailNow there, avoiding unsynchronized writes to err
and eliminating race conditions around err, cont.Stop(), and test termination.
- Line 103: The Ping RPC in the test uses context.Background() which can hang;
replace it by creating a timeout context (e.g., ctx, cancel :=
context.WithTimeout(context.Background(), time.Second*5) with a suitable
duration), defer cancel(), and pass that ctx into client.Ping instead of
context.Background(); update the import list to include time if needed and
ensure the test checks for context deadline errors accordingly.
In `@tests/e2e_http_test.go`:
- Around line 108-109: Replace uses of http.DefaultClient.Do(req) in the e2e
tests with a custom client that has a request timeout to avoid hanging; create a
shared client variable (e.g., client := &http.Client{Timeout: 10 * time.Second})
near the test setup and use client.Do(req) for the three places that currently
call http.DefaultClient.Do(req), and add the time import if missing. Ensure the
timeout value is appropriate for the tests and reuse the same client across
requests.
In `@tests/helpers/helpers.go`:
- Around line 105-114: The retry loop for the destroy RPC never fails the test
if all attempts fail; capture the error from each client.Call (e.g., keep a
lastErr variable) while retrying the destroy call (the code using
client.Call(destroy, pipe, er) and the loop that does "for range 10") and after
the loop, if lastErr != nil call assert.NoError(t, lastErr) or t.Fatalf with the
error so the test fails when all retries are exhausted; ensure you remove the
in-loop assert.NoError that only runs on success and instead check the tracked
error after the loop completes.
- Around line 25-32: The rpcClient function currently dials with
context.Background() and returns an rpc.Client that callers don't close; update
rpcClient to use a context with a 5-second timeout when dialing (e.g.,
context.WithTimeout) and after creating the rpc.Client register a t.Cleanup
closure to call client.Close() so tests (including callPipelines and other
callers) won’t leak connections or hang. Ensure you still return the rpc.Client
from rpcClient and handle any dial errors with require.NoError as before.
---
Nitpick comments:
In `@tests/configs/.rr-jobs-memory.yaml`:
- Around line 14-19: The pool block is missing the standard supervisor
subsection; update the tests/configs/.rr-jobs-memory.yaml by adding a supervisor
block under the existing pool configuration so it matches the .rr*.yaml pattern
(keep pool: num_workers, max_jobs, allocate_timeout, destroy_timeout and add
supervisor with the same keys used across plugins). Locate the pool block
(containing num_workers, max_jobs, allocate_timeout, destroy_timeout) and insert
a supervisor subsection consistent with other .rr*.yaml files so the config
pattern is uniform.
In `@tests/e2e_grpc_test.go`:
- Line 91: Replace the flaky fixed sleep (time.Sleep(time.Second)) with an
active readiness poll: instead of sleeping, loop with a short backoff up to a
timeout and attempt a real readiness check (e.g., grpc.Dial + client health call
or a simple RPC/health endpoint probe) until it succeeds; use context with
deadline and return/fail the test on timeout. Locate the usage of time.Sleep in
the e2e test (tests/e2e_grpc_test.go) and replace it with a helper that
repeatedly tries the gRPC connection/health check and only proceeds when the
server is ready.
In `@tests/mock/logger.go`:
- Around line 71-72: The NamedLogger method should forward the provided name to
the underlying zap logger instead of always returning l.base; update the
function to use the parameter (e.g., change the signature to func (l *Log)
NamedLogger(name string) *zap.Logger) and return l.base.Named(name) so component
labels are preserved (ensure the parameter name matches usage).
In `@tests/mock/observer.go`:
- Around line 186-193: In contextObserver.Write, replace the positional struct
literal LoggedEntry{ent, all} with a named-field literal (e.g.,
LoggedEntry{Entry: ent, Fields: all}) so the creation of LoggedEntry is clear
and robust against field reordering; update the LoggedEntry construction in the
co.logs.add call to use the struct's field names.
- Around line 1-29: This file (package mocklogger) is a near-copy of zap's
observer implementation; either remove the vendored code and import the upstream
package go.uber.org/zap/zaptest/observer where tests use the observer types, or
if you must keep a customized copy, add a top-of-file comment explaining what
was changed and why (documenting differences and maintenance responsibility) and
ensure exported types/functions match the usages in tests so behavior is
explicit; reference the package name mocklogger and any observer-related types
in your tests when making the replacement or documentation.
In `@tests/php_test_files/grpc/src/HealthService.php`:
- Around line 13-24: Both Check and Watch in HealthService duplicate the
HealthCheckResponse construction; extract a private helper (e.g.,
buildServingResponse or createHealthResponse) that returns a HealthCheckResponse
with status set to Serving, and call that helper from both public methods (Check
and Watch) to remove the duplicated response-building logic.
In `@tests/proto/service/service.proto`:
- Around line 3-4: The proto file declares package "service" which doesn't match
its directory layout, triggering Buf's package-directory lint; either update the
package declaration to reflect the directory (e.g., change the package line from
"package service;" to a namespaced package like "package tests.proto.service;")
and adjust option go_package accordingly, or instead modify your Buf
configuration to exclude test protos from this lint rule; locate and edit the
package declaration and the option go_package in the service.proto (symbols: the
"package" statement and the "option go_package" setting) to keep them consistent
with whichever approach you choose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eaf64b9-98de-44fb-ac4e-5f0a63d1f750
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sumtests/proto/service/service.pb.gois excluded by!**/*.pb.gotests/proto/service/service_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (34)
.github/workflows/e2e.yml.gitignorego.modtests/configs/.rr-grpc.yamltests/configs/.rr-http-middleware.yamltests/configs/.rr-http-static.yamltests/configs/.rr-jobs-memory.yamltests/doc.gotests/e2e_grpc_test.gotests/e2e_http_test.gotests/e2e_jobs_test.gotests/go.modtests/helpers/doc.gotests/helpers/helpers.gotests/mock/doc.gotests/mock/logger.gotests/mock/observer.gotests/php_test_files/.gitignoretests/php_test_files/composer.jsontests/php_test_files/grpc/src/EchoService.phptests/php_test_files/grpc/src/GPBMetadata/Service.phptests/php_test_files/grpc/src/Health/HealthCheckRequest.phptests/php_test_files/grpc/src/Health/HealthCheckResponse.phptests/php_test_files/grpc/src/Health/HealthCheckResponse/ServingStatus.phptests/php_test_files/grpc/src/Health/HealthInterface.phptests/php_test_files/grpc/src/HealthService.phptests/php_test_files/grpc/src/Service/EchoInterface.phptests/php_test_files/grpc/src/Service/Message.phptests/php_test_files/grpc/worker-grpc.phptests/php_test_files/http/client.phptests/php_test_files/http/echo.phptests/php_test_files/jobs/jobs_ok.phptests/proto/service/service.prototests/testdata/sample.txt
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2318 +/- ##
=======================================
Coverage 35.37% 35.37%
=======================================
Files 18 18
Lines 851 851
=======================================
Hits 301 301
Misses 511 511
Partials 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/e2e_http_test.go (2)
76-105: Consider extracting the container lifecycle goroutine into a helper.The signal/serve/stop goroutine pattern is duplicated across all three tests. Extracting this into a shared helper would reduce duplication and make tests easier to maintain.
Example helper signature
func runContainer(t *testing.T, cont *endure.Endure, stopCh <-chan struct{}) *sync.WaitGroup { // ... common lifecycle handling ... }Also applies to: 173-202, 271-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e_http_test.go` around lines 76 - 105, The duplicated container lifecycle goroutine (handling ch, sig, and stopCh and calling cont.Stop()) should be extracted into a shared helper to remove repetition; create a helper such as runContainer(t *testing.T, cont *endure.Endure, ch <-chan error, stopCh <-chan struct{}) *sync.WaitGroup (or return a cancel func) that encapsulates the signal.Notify setup, the select loop checking ch, sig and stopCh, and the standardized cont.Stop() + assert.FailNow/assert.Fail handling, then replace the duplicated blocks in tests (the blocks referencing sig, ch, stopCh, wg, cont) with calls to runContainer so each test invokes the helper with its cont, channels, and t.
107-107: Consider replacingtime.Sleepwith a readiness check.Using a fixed 1-second sleep for server readiness can cause flakiness if the server takes longer to initialize under load or in CI environments. A retry loop with backoff polling an endpoint (e.g., health check or the actual endpoint) would be more robust.
Example readiness helper
func waitForServer(t *testing.T, url string, timeout time.Duration) { t.Helper() client := &http.Client{Timeout: 500 * time.Millisecond} deadline := time.Now().Add(timeout) for time.Now().Before(deadline) { resp, err := client.Get(url) if err == nil { _ = resp.Body.Close() return } time.Sleep(100 * time.Millisecond) } t.Fatalf("server at %s not ready within %v", url, timeout) }Also applies to: 204-204, 302-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e_http_test.go` at line 107, Replace the fragile time.Sleep call in tests/e2e_http_test.go with a readiness check that polls the running server; add a helper like waitForServer(t, url, timeout) (or reuse the provided example) and call it instead of time.Sleep in the test functions that currently use time.Sleep at lines referenced (e.g., where time.Sleep(time.Second) appears and the other occurrences at lines ~204 and ~302), ensuring the helper uses a short HTTP GET loop with a deadline/backoff and fails the test via t.Fatalf() if the server never becomes ready.tests/helpers/helpers.go (1)
49-52: Consider direct slice assignment instead of getter method.The
GetPipelines()method returns the same slice, so modifying it works, but direct assignment toPipelinesis more idiomatic and clearer.Suggested simplification
- pipe := &jobsProto.Pipelines{Pipelines: make([]string, len(pipes))} - for i := range pipes { - pipe.GetPipelines()[i] = pipes[i] - } + pipe := &jobsProto.Pipelines{Pipelines: pipes}This applies to both
callPipelines(lines 49-52) andDestroyPipelines(lines 108-111).Also applies to: 108-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/helpers/helpers.go` around lines 49 - 52, Replace the use of the getter and per-index writes with direct assignment to the struct field: in callPipelines and DestroyPipelines, construct the payload as pipe := &jobsProto.Pipelines{Pipelines: append([]string(nil), pipes...)} (or pipe := &jobsProto.Pipelines{Pipelines: make([]string, len(pipes)); copy(pipe.Pipelines, pipes)}) so you assign the slice to the Pipelines field directly instead of calling pipe.GetPipelines()[i] in a loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/go.mod`:
- Around line 132-138: Add the missing exclude for
github.com/olekukonko/tablewriter v1.1.1 to the tests module's exclude block so
it matches the root go.mod; locate the exclude (...) stanza in tests/go.mod and
append an exclude entry for "github.com/olekukonko/tablewriter v1.1.1" to ensure
the test module cannot resolve that banned version in workspace mode.
---
Nitpick comments:
In `@tests/e2e_http_test.go`:
- Around line 76-105: The duplicated container lifecycle goroutine (handling ch,
sig, and stopCh and calling cont.Stop()) should be extracted into a shared
helper to remove repetition; create a helper such as runContainer(t *testing.T,
cont *endure.Endure, ch <-chan error, stopCh <-chan struct{}) *sync.WaitGroup
(or return a cancel func) that encapsulates the signal.Notify setup, the select
loop checking ch, sig and stopCh, and the standardized cont.Stop() +
assert.FailNow/assert.Fail handling, then replace the duplicated blocks in tests
(the blocks referencing sig, ch, stopCh, wg, cont) with calls to runContainer so
each test invokes the helper with its cont, channels, and t.
- Line 107: Replace the fragile time.Sleep call in tests/e2e_http_test.go with a
readiness check that polls the running server; add a helper like
waitForServer(t, url, timeout) (or reuse the provided example) and call it
instead of time.Sleep in the test functions that currently use time.Sleep at
lines referenced (e.g., where time.Sleep(time.Second) appears and the other
occurrences at lines ~204 and ~302), ensuring the helper uses a short HTTP GET
loop with a deadline/backoff and fails the test via t.Fatalf() if the server
never becomes ready.
In `@tests/helpers/helpers.go`:
- Around line 49-52: Replace the use of the getter and per-index writes with
direct assignment to the struct field: in callPipelines and DestroyPipelines,
construct the payload as pipe := &jobsProto.Pipelines{Pipelines:
append([]string(nil), pipes...)} (or pipe := &jobsProto.Pipelines{Pipelines:
make([]string, len(pipes)); copy(pipe.Pipelines, pipes)}) so you assign the
slice to the Pipelines field directly instead of calling pipe.GetPipelines()[i]
in a loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a2ed88f-ff7c-4a7a-a3e7-67fb8418e688
⛔ Files ignored due to path filters (2)
go.work.sumis excluded by!**/*.sumtests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
tests/e2e_grpc_test.gotests/e2e_http_test.gotests/e2e_jobs_test.gotests/go.modtests/helpers/helpers.gotests/mock/logger.gotests/mock/observer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e_jobs_test.go
- tests/e2e_grpc_test.go
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Member
Author
Code reviewFound 1 issue:
Lines 1 to 3 in 34dc4b0 Lines 26 to 28 in 34dc4b0 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e_jobs_test.go`:
- Around line 154-177: The background goroutine inside the wg.Go block uses
assert.Fail/assert.FailNow directly and must be refactored to send errors to a
dedicated error channel for the test (e.g., errCh := make(chan error, 1))
instead of calling assertions inside the goroutine; update the select cases that
currently call assert.Fail/assert.FailNow or cont.Stop() to send the encountered
error (or wrap stopErr) to errCh and then return, and in the main test goroutine
select/receive from errCh to call t.Fatalf/t.FailNow so all test assertions run
on the main goroutine; keep references to the existing symbols (ch, sig, stopCh,
cont.Stop) and ensure the goroutine returns after sending to errCh to avoid
races and leaked goroutines.
- Around line 12-14: Replace the relative test imports with module-qualified
imports: change the alias import mocklogger "tests/mock" to mocklogger
"github.com/roadrunner-server/roadrunner/v2025/tests/mock" and change the
helpers import "tests/helpers" to
"github.com/roadrunner-server/roadrunner/v2025/tests/helpers" so the package
uses the full module path for proper module resolution.
- Around line 70-93: The background goroutine started via wg.Go should not call
assert.Fail/assert.FailNow or access shared fields like e.Error directly;
instead create a dedicated error channel (e.g., errCh := make(chan error, 1)),
replace all assert.Fail/assert.FailNow and direct cont.Stop error handling
inside the wg.Go lambda by sending the error (or wrapped error including
cont.Stop failures) into errCh (ensure you read e.Error into a local variable
before sending to avoid races), then in the main test goroutine after wg.Wait()
check errCh (non-blocking or close it and range) and call t.Fatal/t.Fatalf or
assert there to fail the test; also ensure cont.Stop is still invoked but its
error is propagated to errCh rather than failing inside the background
goroutine.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ed3cf0b-5883-42e1-a488-d1a0c232118f
📒 Files selected for processing (4)
tests/configs/.rr-grpc-otel.yamltests/configs/.rr-jobs-memory-otel.yamltests/e2e_grpc_test.gotests/e2e_jobs_test.go
Comment on lines
+70
to
+93
| wg.Go(func() { | ||
| for { | ||
| select { | ||
| case e := <-ch: | ||
| assert.Fail(t, "error", e.Error.Error()) | ||
| stopErr := cont.Stop() | ||
| if stopErr != nil { | ||
| assert.FailNow(t, "error", stopErr.Error()) | ||
| } | ||
| case <-sig: | ||
| stopErr := cont.Stop() | ||
| if stopErr != nil { | ||
| assert.FailNow(t, "error", stopErr.Error()) | ||
| } | ||
| return | ||
| case <-stopCh: | ||
| stopErr := cont.Stop() | ||
| if stopErr != nil { | ||
| assert.FailNow(t, "error", stopErr.Error()) | ||
| } | ||
| return | ||
| } | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Avoid assert.Fail/assert.FailNow in background goroutines.
assert.FailNow called from a non-test goroutine only terminates that goroutine, not the test. The unsynchronized access to e.Error also creates a race condition detectable with -race.
Consider refactoring to use an error channel:
Proposed refactor pattern
+ errCh := make(chan error, 1)
wg := &sync.WaitGroup{}
wg.Go(func() {
for {
select {
case e := <-ch:
- assert.Fail(t, "error", e.Error.Error())
- stopErr := cont.Stop()
- if stopErr != nil {
- assert.FailNow(t, "error", stopErr.Error())
- }
+ errCh <- e.Error
+ _ = cont.Stop()
+ return
case <-sig:
- stopErr := cont.Stop()
- if stopErr != nil {
- assert.FailNow(t, "error", stopErr.Error())
- }
+ _ = cont.Stop()
return
case <-stopCh:
- stopErr := cont.Stop()
- if stopErr != nil {
- assert.FailNow(t, "error", stopErr.Error())
- }
+ _ = cont.Stop()
return
}
}
})Then check errCh after wg.Wait() in the main test goroutine.
As per coding guidelines, tests should be run with the -race flag to detect race conditions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e_jobs_test.go` around lines 70 - 93, The background goroutine
started via wg.Go should not call assert.Fail/assert.FailNow or access shared
fields like e.Error directly; instead create a dedicated error channel (e.g.,
errCh := make(chan error, 1)), replace all assert.Fail/assert.FailNow and direct
cont.Stop error handling inside the wg.Go lambda by sending the error (or
wrapped error including cont.Stop failures) into errCh (ensure you read e.Error
into a local variable before sending to avoid races), then in the main test
goroutine after wg.Wait() check errCh (non-blocking or close it and range) and
call t.Fatal/t.Fatalf or assert there to fail the test; also ensure cont.Stop is
still invoked but its error is propagated to errCh rather than failing inside
the background goroutine.
Comment on lines
+154
to
+177
| wg.Go(func() { | ||
| for { | ||
| select { | ||
| case e := <-ch: | ||
| assert.Fail(t, "error", e.Error.Error()) | ||
| stopErr := cont.Stop() | ||
| if stopErr != nil { | ||
| assert.FailNow(t, "error", stopErr.Error()) | ||
| } | ||
| case <-sig: | ||
| stopErr := cont.Stop() | ||
| if stopErr != nil { | ||
| assert.FailNow(t, "error", stopErr.Error()) | ||
| } | ||
| return | ||
| case <-stopCh: | ||
| stopErr := cont.Stop() | ||
| if stopErr != nil { | ||
| assert.FailNow(t, "error", stopErr.Error()) | ||
| } | ||
| return | ||
| } | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
Same goroutine pattern issue as TestJobsInMemory.
This background goroutine has the same assert.Fail/assert.FailNow issue. Apply the same error channel refactor for consistency and race-safety.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e_jobs_test.go` around lines 154 - 177, The background goroutine
inside the wg.Go block uses assert.Fail/assert.FailNow directly and must be
refactored to send errors to a dedicated error channel for the test (e.g., errCh
:= make(chan error, 1)) instead of calling assertions inside the goroutine;
update the select cases that currently call assert.Fail/assert.FailNow or
cont.Stop() to send the encountered error (or wrap stopErr) to errCh and then
return, and in the main test goroutine select/receive from errCh to call
t.Fatalf/t.FailNow so all test assertions run on the main goroutine; keep
references to the existing symbols (ch, sig, stopCh, cont.Stop) and ensure the
goroutine returns after sending to errCh to avoid races and leaked goroutines.
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
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.
Reason for This PR
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Tests
Chores