chore: overhaul the test suite - #147
Merged
Merged
Conversation
Replace the per-test endure boilerplate with a Start helper plus StartExpectInitError and StartExpectServeError, and split the 797-line file by theme: relay modes, on_init behaviour and rejected configs. TestOnInitMetrics booted the on_init script that declares a collector over rpc and then asserted nothing, so a script that declared nothing passed. Scrape the metrics endpoint and require the collector is there. TestAppNoAppSectionInConfig loaded .rr-wrong-command.yaml, making it a duplicate of TestAppWrongCommand, and the .rr-no-app-section.yaml it was named after has a server section, so the name never matched either file. Drop the duplicate and the misnamed config. The remaining env config was unreferenced too. Point it at a worker that echoes an environment variable and assert both a plain value and one built through expansion reach the worker process. Registering http and prometheus is not needed by any of these configs, so the tests module no longer depends on them.
coverpkg named the root package only, so subpackages went unmeasured, and the test target was ./ rather than ./... Drop -failfast and fail the codecov job when the merged summary maps to no plugin source.
The script sent the collector as a plain array, so the metrics plugin decoded the type as unspecified and rejected the call with 'unknown collector type'. It has been broken for a while; the test only booted the config and asserted nothing, so nobody saw it. Use the metrics sdk, which builds the protobuf the plugin expects, and poll the exporter for the collector rather than scraping once. Also give the config a runnable worker command so the log is not full of unrelated spawn failures.
There was a problem hiding this comment.
Pull request overview
This PR overhauls the tests module’s e2e suite by splitting the former monolithic test file into themed suites and centralizing container boot/teardown boilerplate in a shared helper. It also strengthens coverage by turning previously “assert-nothing” tests into ones that verify observable outcomes (metrics scraping; server.env propagation), and adjusts CI coverage collection to measure subpackages and enforce a minimal “profile maps to source” guard.
Changes:
- Split
tests/server_plugin_test.gointo themed test files (relay_test.go,oninit_test.go,errors_test.go) and addedtests/helpers/rr.goto remove duplicated Endure setup. - Added a new env worker (
php_test_files/env.php) +FooEnvfixture and updated.rr-env.yamlto assert thatserver.envreaches workers (including${...}expansion). - Updated CI workflow coverage collection (
coverpkg=./.../.../server/v6/...), removed-failfast, and added a coverage-summary guardrail.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/server_plugin_test.go | Deleted the old monolithic test suite in favor of themed files + shared helper. |
| tests/relay_test.go | New relay-focused tests using the shared Start helper. |
| tests/oninit_test.go | New on_init-focused tests; adds metrics endpoint scrape assertion. |
| tests/errors_test.go | New config/boot error tests using StartExpect* helpers. |
| tests/helpers/rr.go | New shared Endure container bootstrap/teardown helper + probe options. |
| tests/plugin_tcp.go | Removes unused op constant. |
| tests/plugin_sockets.go | Removes unused op constant. |
| tests/plugin_pool_with_options.go | Removes unused op constant. |
| tests/plugin_pipes.go | Removes unused op constant. |
| tests/plugin_env.go | New fixture plugin that asserts worker environment variables. |
| tests/php_test_files/env.php | New PHP worker that echoes a requested env var value. |
| tests/configs/.rr-no-app-section.yaml | Removed unused/misleading config fixture. |
| tests/configs/.rr-env.yaml | Repurposed env fixture to drive env propagation test; lowered log level. |
| tests/go.mod | Drops unused deps (http/prometheus) and bumps toolchain patch. |
| tests/go.sum | Prunes sums corresponding to removed deps. |
| .github/workflows/linux.yml | Improves coverage measurement and adds a coverage mapping guard. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The collector assertion cannot pass yet. metrics/v6 beta.5 resolves api-go beta.13, the connect-era protos, while the php sdk is on the v1 line, so Declare arrives empty: declaring new metric name="" type=COLLECTOR_TYPE_UNSPECIFIED namespace="" Assert the exporter is serving instead, and record in the test what to switch back to once the plugin betas are retagged against beta.14. The sdk rewrite in the previous commit stands: the payload was also built the old way, so both had to change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #147 +/- ##
==========================================
+ Coverage 79.02% 80.97% +1.95%
==========================================
Files 4 4
Lines 205 205
==========================================
+ Hits 162 166 +4
+ Misses 25 22 -3
+ Partials 18 17 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
All three were untested. UID and GID have a nil guard for the case where no run-as user is configured, which nothing exercised.
InitDefaults, newCommand, Write and createProcess had no unit tests, so the only thing exercising them was a booted container. Cover the defaulting rules, the writer that forwards on_init output to the logger, and the env handling: keys are uppercased, values expanded, and config env is appended after the OS environment so it wins.
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.
Wave 2 overhaul. 797 lines and 17 tests, each opening with the same ~45 lines of endure boilerplate.
Two tests asserted nothing meaningful.
TestOnInitMetricsran theon_initscript that declares a prometheus collector over rpc, then booted and stopped without checking anything — a script that declared nothing would have passed. It now scrapes the metrics endpoint and requiresfoo_bar_testto be present.TestAppNoAppSectionInConfigloadedconfigs/.rr-wrong-command.yaml, which made it a duplicate ofTestAppWrongCommand. The.rr-no-app-section.yamlit was named after does have aserversection, so the name never described either file. Dropped the duplicate test and the misnamed config.Two config fixtures were dead. Both env configs were unreferenced. One now drives a new
env.phpworker that echoes a named environment variable, so the suite provesserver.envreaches the worker process — both a plain value and one built through${...}expansion.Split by theme:
relay_test.go(pipes, big response, sockets, tcp, pool-with-options, env),oninit_test.go(tcp and sockets with on_init, fast close, error, timeout, metrics),errors_test.go(rejected configs). The helper gainedStartExpectInitErrorandStartExpectServeErrorso the error cases stay one line each.httpandprometheusare not needed by any of these configs, so the tests module no longer depends on them — which also drops anhttp beta.8pin from this module.On verification: I could only run part of this locally. The socket and tcp relay tests and the on_init metrics script need PHP's
ext-sockets, which this machine lacks — the on_init script fails withassert(\extension_loaded('sockets'))and the relay tests hang waiting for a worker that never connects. I confirmed this is pre-existing rather than something I introduced: the originalTestAppTCPhangs identically here on an unmodified checkout. The ten tests that do not need sockets all pass locally; CI has the extension and covers the rest.CI also gains the unit tier over
./...(coverpkg named only the root package, so subpackages went unmeasured), loses-failfast, and gains the coverage guard.