You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/08/13 18:34:32 UTC
[PR] Add pytest replay test framework [trafficserver]
bneradt opened a new pull request, #13545:
URL: https://github.com/apache/trafficserver/pull/13545
## Summary
- collect `.test.yaml` Proxy Verifier replays as isolated pytest items
- provide ATS-specific DNS, Proxy Verifier, Traffic Server, log, metric, capability-gate, and parallel-worker lifecycle support
- convert all 96 `ATSReplayTest` registrations to direct collection while retaining bespoke AuTests
- run pytest replays from the existing CMake `autest` targets and document the workflow
This supersedes #13035 by moving direct replay-file collection out of the stalled AuTest extension path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5319770129
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5736600706
@bryancall Rebased onto current master, `281b608361`, and force-pushed `57c0465079`. The branch now contains that master revision, with the merge conflict resolved.
The rebase carried over the updated per-server metric coverage from #13666: the `current_connection.max` name, aggregation mode 3 for sums plus max, exclusion of hidden per-group metrics, and the new runtime retraction case. The Uranium scenario verifies that previously published group metrics disappear when switching from mode 0 to max-only mode 2, and that hostname sums are absent. It waits for the behavior to take effect while recreating drained connection groups. I also adapted master's new process-lifetime review guidance to Uranium's fixture ownership.
Validation on the rebased tree in asfats5: ATS build/install and the full formatting target passed; all 73 framework tests passed; all 145 selected C++ tests passed through CTest; and all three origin-connection scenarios passed with `-n 4`, including the expanded per-server test. These are local results; the new CI run still needs to complete.
The substantive conversion delta from the previous pushed head is in `test_per_server_connection_max.py` and `.github/copilot-instructions.md`; the remaining changes between those heads come from master. Ready for your delta re-review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#discussion_r4157580325
##########
ci/rat-exclude.txt:
##########
@@ -76,8 +76,8 @@ blib/**
**/systemtap/**
**/swoc/**
**/highway/**
-tests/gold_tests/autest-site/min_cfg
-tests/gold_tests/h2/rules/huge_resp_hdrs.conf
+tests/uranium_tests/autest-site/min_cfg
Review Comment:
This exclusion now points at `tests/uranium_tests/autest-site/min_cfg`, but that directory does not exist; the new unlicensed fixture files are under `tests/tools/uranium/min_cfg`. RAT will therefore scan these files and can fail the license check. Point the exclusion at the actual directory.
##########
CMakeLists.txt:
##########
@@ -162,8 +162,11 @@ option(ENABLE_DISK_FAILURE_TESTS "Build disk failure tests (enables AIO fault in
if(ENABLE_DISK_FAILURE_TESTS)
add_compile_definitions("AIO_FAULT_INJECTION")
endif()
-option(ENABLE_AUTEST "Setup autest (default OFF)")
-option(ENABLE_AUTEST_UDS "Setup autest with curl using UDS (default OFF)")
+option(ENABLE_URTEST "Set up Uranium tests (default OFF)")
+option(ENABLE_URTEST_UDS "Set up Uranium tests with curl using UDS (default OFF)")
+if(DEFINED ENABLE_AUTEST OR DEFINED ENABLE_AUTEST_UDS)
+ message(FATAL_ERROR "ENABLE_AUTEST and ENABLE_AUTEST_UDS were removed; use ENABLE_URTEST and ENABLE_URTEST_UDS")
Review Comment:
This fatal guard now breaks the existing `contrib/docker/ubuntu/resolute/Dockerfile`, which still configures CMake with `-DENABLE_AUTEST=OFF`. Update that image recipe (and any other remaining callers) to the new `ENABLE_URTEST` option before making the legacy variable an unconditional configuration error.
This issue also appears on line 864 of the same file.
##########
doc/developer-guide/api/functions/TSHttpHookAdd.en.rst:
##########
@@ -146,7 +146,7 @@ transaction hooks::
}
For more example code using hooks, see the test_hooks plugin in tests/tools/plugins (used by the test_hooks.test.py
-Gold test).
+Uranium test).
Review Comment:
The edit leaves the sentence as “used by the test_hooks.test.py Uranium test”, which is grammatically malformed and still contains the old test filename. Update the sentence to name the migrated test correctly.
##########
doc/developer-guide/testing/config-reload-ext.en.rst:
##########
@@ -27,7 +27,7 @@ replace the legacy pattern of fire-and-forget ``traffic_ctl config reload``
followed by log grepping with a deterministic, structured approach.
The extension is loaded automatically from
-``tests/gold_tests/autest-site/config_reload.test.ext``.
+``tests/uranium_tests/autest-site/config_reload.test.ext``.
Review Comment:
This documentation now points to `tests/uranium_tests/autest-site/config_reload.test.ext`, but that file and directory are absent from the tree. The page therefore documents a loader path that cannot work after the AuTest cutover; remove or rewrite this extension section around the actual Uranium reload tests.
##########
plugins/experimental/jax_fingerprint/README:
##########
@@ -158,6 +158,6 @@ Unit tests vs. AuTest
algorithm logic. Built when BUILD_TESTING is on; run via ctest or the
test_jax binary directly.
- * End-to-end AuTests live under `tests/gold_tests/pluginTest/jax_fingerprint/`
+ * End-to-end AuTests live under `tests/uranium_tests/pluginTest/jax_fingerprint/`
Review Comment:
This still labels the migrated suite as “AuTests” even though it now lives under Uranium and the PR removes the AuTest backend. Rename the reference so this README does not direct readers to a nonexistent test system.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5763813800
@bryancall Thanks for the detailed assertion-by-assertion review. I rebased onto master `7741a2b237`, converted the newly added cache tests, and retained both the busy and quiet Lua shutdown scenarios added upstream.
The review changes are:
1. Native ATS teardown now fails on an unexpected exit, including status 0 and signal exits. Explicit stop/kill and validated one-shot waits remain supported. A missing diagnostic destination fails too; renamed logs and stdout/stderr are checked at their configured destination. Regression tests cover these cases.
2. Background-fetch and the affected abuse-shield negative assertions now read the complete output after stopping ATS. Reload polling requires the reload to leave `in_progress` before accepting its assertions.
3. Replay `file_checks` now implements `excludes`, rejects unknown keys (including nested match keys), and enforces `line_count_min` after its polling deadline. Regression tests exercise rejected assertions and invalid manifests.
4. Every abuse-shield instance now verifies the startup rule count from its configured rules before sending traffic.
5–8. Embedded replays use separate child directories under the native scenario, including distinct directories for repeated calls and variants. Collision diagnostics are printed before raising so they remain visible with xdist. A controller-held exclusive sandbox-root lock rejects concurrent sessions before they can reset the port counter or delete artifacts; the diagnostic tells callers to choose a different `--sandbox`.
On point 7, I deliberately kept the full readable test names requested for this branch instead of restoring truncation. Both JSON-RPC and HTTP Unix sockets use short paths independently of those names; a regression test binds both actual sockets under long sandbox/process names. Unrepresentable filesystem components now fail explicitly. This does not impose a blanket limit for arbitrary future path-limited artifacts; those would need the same short-path treatment.
9–11. The record-triggered reload uses bounded polling, with a fresh trigger marker so an earlier reload cannot satisfy it. `rpc-greet` rejects all returned errors. The abuse-shield checks again require `for IP=`, and the shared-log-interval check requires exactly `127.0.0.1`.
12. I retained the slice diagnostic alternation: it is already present in the original AuTest, together with the timing explanation ([original lines 295–304](https://github.com/apache/trafficserver/blob/7741a2b237/tests/gold_tests/pluginTest/slice/slice_stale_generation.test.py#L295-L304)). An abort can arrive before or after the response header. The converted test still requires client failure, failed transaction identity, and the specific block/ETag mismatch diagnostics. I brought the explanatory comment across as well.
13. Runroot verification no longer recursively changes permissions. It verifies the copied tree as the initializing user with `--with-user`, leaving copied file modes untouched, and adds a deliberately unwritable log to require verification to fail.
14. Only the locked session controller initializes the port counter. Missing/corrupt/exhausted state produces a path-bearing error; only `EADDRINUSE` retries, while other bind errors report the address and OS error. Regression tests cover these cases.
15. The two ineffective native `disable_log_checks` flags were removed.
I left the inherited `idle_connections.py` behavior unchanged in this conversion. The connection-rate scenario additionally requires the rule-match log, blocked-action metric, and rejected-connection metric, so a single successful connection cannot by itself make that scenario pass. The helper's partial-success reporting remains a separate follow-up.
Validation on the rebased branch:
- Build/install and the full CMake formatting target succeeded.
- All 1,116 CTest cases passed.
- All 103 Uranium framework tests passed.
- The final eight-worker selection covering cache tests and review-affected scenarios finished with **143 passed, 4 skipped**. This includes the new upstream cache replay, both Lua shutdown paths, renamed/stdout/stderr diagnostics, reload tests, runroot verification, and abuse-shield scenarios.
- An earlier full eight-worker run finished with 1,260 passed, 39 skipped, and one teardown error in the renamed/redirected-log scenario. That exposed the diagnostic-destination edge case described above; the fix is covered by both regression tests and the successful final scenario run. I have not rerun the entire suite after the final rebase.
There are no inline review threads to resolve; points 7 and 12 above are explicit responses rather than changes to the requested naming policy or the original slice contract.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5349255287
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5334843406
Thanks for the detailed review. I addressed the feedback and force-pushed the amended commit.
- Removed both unrelated production changes from this PR. The max-stale percentage feature landed separately in #13547. The converted certificate-update scenario is now a strict expected failure for the pre-existing `TSSslClientCertUpdate` defect, which can be fixed separately.
- Updated the Jenkins, coverage, and regression scripts to invoke Uranium and use pytest's `-k` and `-n` options.
- Kept this as an intentional clean cutover rather than an AuTest compatibility shim. Removed AuTest CMake variables now produce explicit migration errors, and I updated the PR description accordingly.
- Changed automatic execution to avoid nested Docker in any detected container, while retaining the explicit Docker override flags.
- Changed metric checks to poll until their deadline.
- Added explicit assertions for missing actual, gold, and diagnostic files.
- Renamed the framework's local `RuntimeError` to `RuntimeConfigError`.
I also normalized direct replay placement, fixed the stale documentation includes, and reran the relevant validation: the documentation build passes with warnings treated as errors, all 371 replay variants passed or reached expected capability skips, and the 51 framework unit tests pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5336335432
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5344363665
I benchmarked this migration on dedicated hardware rather than reasoning about it, because a harness swap of this size deserves numbers. Summary up front: **Uranium is 7.4x faster at 32-way parallelism, loses no code coverage, uses less memory, and has fewer flaky tests.** Details and methodology below, including the parts that do not favour the new harness.
## Methodology
Two hosts, both 32 hardware threads, 30 GB RAM, Fedora, gcc 16.1.1, Proxy Verifier v3.1.3 on both sides (verified identical, checksum `342286244d...`). Timing on one host, coverage on the other, and each host ran **both** harnesses so every comparison is within-host.
- master at `a2ea029215`, this branch at `7fcbc9d69`
- Python pinned identically for every run, dependencies resolved from the checked-in `tests/uv.lock`
- Timing used Release builds; coverage used separate gcov builds, since `-O0` plus instrumentation distorts wall clock by about 1.5x
- Coverage counters were reset before each run and **only the integration suite** was measured. Unit tests are identical on both sides and including them would dilute exactly the delta in question.
**The proxy binary is the same on both sides.** I checked this rather than assuming it, because the whole comparison depends on it. After normalizing embedded build paths, both binaries disassemble to **1,341,244 instructions** and the instruction streams are identical once addresses are masked. The residual byte differences are relocation displacements from embedded path and timestamp strings of different lengths, spread across 1,265 unrelated symbols in single-byte runs. So every number below is harness overhead, not a proxy change.
## Performance
Full suite, same host, same binary:
| Workers | AuTest | Uranium | Speedup |
|---|---|---|---|
| 4 | 3230s | 914s | 3.5x |
| 8 | 2254s | 497s | 4.5x |
| 16 | 1749s | 293s | 6.0x |
| 32 | 1591s, 1596s | 215s, 215s, 202s | **7.4x** |
The gap widens with worker count, which points at the mechanism:
| | AuTest | Uranium |
|---|---|---|
| Total work | 4873s of worker time | 3899s of test time |
| Wall clock | 1031s | 215s |
| **Effective parallelism** | **4.7x** | **18.1x** |
AuTest's worker durations at `-j32` were min 113s, median 155s, **max 889s**. One worker set the wall clock while roughly 30 sat idle behind it.
**The cause is that AuTest's load balancing never engages.** `autest-parallel.py` has an LPT balancer that reads `<sandbox>/test-timings.json`, but that file is never written: the save at line 1135 is gated on `tests_timed > 0` and the per-worker timing dictionaries come back empty. Every run logs `Using round-robin partitioning` verbatim, and I confirmed no timings file exists after seven runs. This is not a cold-start artifact, it is the steady state. Worth knowing regardless of this PR's outcome, since it means the current suite is leaving most of the machine idle.
Uranium's per-test distribution for reference: p50 1.13s, p95 9.17s, max 90.6s.
## Code coverage
gcov plus gcovr 8.6, identical flags and exclusions on both sides, integration suite only:
| | AuTest | Uranium | Delta |
|---|---|---|---|
| Lines | 51.0% (96,576 / 189,482) | **51.1%** (96,784 / 189,482) | +208 |
| Functions | 60.7% (11,376 / 18,726) | **60.8%** (11,392 / 18,726) | +16 |
| Branches | 25.6% (77,497 / 302,664) | 25.6% (77,583 / 302,664) | +86 |
**No coverage is lost.** Uranium is fractionally ahead on all three measures. This is the number that matters most, since a 7.4x speedup naturally raises the question of whether the suite is simply doing less. It is not. The denominators are identical on both sides, which independently corroborates the binary-equivalence check above.
## Memory
Peak resident memory across the whole process tree:
| Workers | AuTest | Uranium |
|---|---|---|
| 4 | 3.78 GB | 3.66 GB |
| 8 | 5.48 GB | 4.52 GB |
| 16 | 7.47 GB | 5.69 GB |
| 32 | 11.0 to 12.26 GB | 9.64 to 10.03 GB |
Uranium uses less memory at every level, with the gap widening to about 31% at 16 workers. Neither harness came close to exhausting 30 GB.
Caveat worth stating: these are summed RSS across all processes, which double counts pages shared between the many concurrent Traffic Server instances, so treat them as upper bounds. The idle baseline was 0.97 GB, so roughly 92% of each figure is genuine workload. I am re-measuring with proportional set size to remove the double counting and will follow up if it changes the picture.
## Flakiness
Seven AuTest runs and six Uranium runs, across all parallelism levels:
| Harness | Test | Failed in | Verdict |
|---|---|---|---|
| AuTest | `per_client_connection_max` | 7/7 | deterministic |
| AuTest | `cripts` | 7/7 | deterministic |
| AuTest | `h2_malformed_request_logging` | 2/7 | **flaky** |
| AuTest | `session_id` | 1/7 | **flaky** |
| Uranium | `test_all_bespoke_tests_are_available_to_pytest` | 6/6 | deterministic |
| Uranium | `log_mstsms` | 2/6 | **flaky** |
Two observations that only a repeated run surfaces.
**A flaky test was distorting the timing.** AuTest at `-j32` came in bimodal: 1031s, 1039s, then 1591s, 1596s. The fast runs are the ones where `h2_malformed_request_logging` **failed**; the slow ones are where it **passed**. Its success path costs roughly 550 seconds more than its failure path. A single measurement would have reported a figure that flattered AuTest by 35%.
**AuTest does not run a deterministic set of tests.** Executed totals across runs were 586, 586, 586, 586, 586, 583, 581. At `-j8` three tests silently disappear and at `-j4` five do, with no diagnostic. Uranium collected exactly **1207 items in all six runs** with no drift. For a suite whose purpose is regression detection, that difference matters as much as the speed.
## Test inventory
I mapped all 564 master `*.test.py` files against the branch. 388 matched by directory and stem; I resolved the remaining 176 individually by reading each successor rather than inferring from names.
**Exactly one test has no successor: `tests/gold_tests/cache/cache-write-lock-contention.test.py`.** Severity is low since it was already `SkipUnless(RUN_CACHE_CONTENTION_TEST=1)`, so CI signal is unchanged, but the scenario is gone and the gate variable is now dead plumbing: `tests/tools/uranium/runner.py` still forwards `RUN_CACHE_CONTENTION_TEST` into the container and nothing reads it. Either restore the scenario or drop the plumbing.
Everything else is verified consolidation, and the counts hold up: the 18 `tls_hooks` files became a 17-entry parametrization plus one function, the 7 `cont_schedule` files became a 7-entry parametrization, 38 `txn_box` tests became 35 manifests with three merged pairs, and `cache` grew from 39 to 70. I also confirmed the master merge converted the two tests #13547 added after my baseline rather than dropping them.
## Other findings
- **`ci/coverage` cannot collect coverage as written.** It calls the build-tree `./urtest.sh -n "$NPROCS"` without `--no-run-in-docker`. Since `choose_docker_mode()` returns `not is_container()`, on any non-container host that shells out to Docker and discards the gcov-instrumented build the script just made. I had to bypass this script to get the coverage numbers above.
- **`ci/regression` and `ci/jenkins/bin/autest.sh` no longer exercise the tree they build.** Source-mode `urtest.sh` always runs its own `cmake --preset urtest` into `build-urtest-container`, so `$DSTROOT` and `${INSTALL}` are ignored and the build above the test call is dead weight.
- **`tests/uv.lock` never reaches the build tree.** `tests/CMakeLists.txt` copies only `pyproject.toml`, but `runner.py` runs `uv --project <build>/tests`, so the vetted lock is unused and every first run resolves fresh against PyPI. This bit me concretely: without an explicit pin, `uv` selected **Python 3.12.12** on a host whose system interpreter is 3.14. Copying `uv.lock` alongside `pyproject.toml` is a one-line fix and makes runs reproducible.
- **`test_all_bespoke_tests_are_available_to_pytest` fails deterministically**, in all six runs. A framework self-check reporting that the declared inventory does not match what pytest collects is worth resolving before this lands, since it is the test that would otherwise catch a conversion gap.
- **The documentation contradicts the code on Docker detection.** `uranium-tests.en.rst` describes a two-condition rule (container **and** Fedora 44); `choose_docker_mode()` checks only `is_container()`.
- **`if(DEFINED ENABLE_AUTEST)` fires even for `-DENABLE_AUTEST=OFF`**, which will surprise anyone carrying that flag in a script. The companion guard for `AUTEST_SANDBOX` / `AUTEST_OPTIONS` / `PYTEST_OPTIONS` sits inside `if(ENABLE_URTEST)`, so it misses the common case.
- **Replay manifests cannot be marked serial.** `_is_serial_test` matches only `.py` paths and `ReplayItem.runtest()` always takes the shared lock, so a `.test.yaml` listed in `serial_tests.txt` would be silently ignored. Latent today since the file lists only `.py` entries, but it is a trap for whoever first needs an exclusive replay.
- **Sharding moved into the repository, which the PR body understates.** `SHARD` and `SHARDCNT` appear nowhere on master; the 1of4 through 4of4 split is external Jenkins configuration. This PR implements sharding in-repo as a modulo stripe over sorted node IDs, so any existing per-shard intuition about which tests land where is void, and `ci/jenkins/bin/autest.sh` now passes no shard flags at all. Worth confirming the Jenkins job definitions move in lockstep.
## Overall
The performance and determinism case is strong and I would not have predicted the margin. The two things I would want resolved before this leaves draft are the `ci/coverage` Docker bug, since it silently disables coverage collection, and the deterministic `test_all_bespoke_tests_are_available_to_pytest` failure. The single lost test and the `uv.lock` plumbing are small and easy.
Happy to share the raw logs, per-test timing data, or the gcovr HTML reports for either side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall merged PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5351126038
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5361038330
[approve ci ubuntu autest 3]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-5482014182
I got the Uranium suite running natively on macOS (no container) and wired it into neovim's `neotest-python`, and hit a handful of small friction points along the way. All of these are optional polish rather than blockers — the suite itself worked well: **69 passed / 2 failed / 4 skipped** across `uranium_tests/cache` and `uranium_tests/basic` on a native darwin-arm64 build.
## The issue I'd most like to see fixed: unresolved imports in editors
Opening any `test_*.py` under `tests/uranium_tests/` in an LSP-backed editor reports `Import "pytest" could not be resolved`, because pyright has no idea where the environment lives. Anyone opening a Uranium test in VS Code, neovim, or PyCharm gets import noise until they work this out for themselves.
The support-package paths are repo-invariant, so most of it can just be checked in. Adding this to `tests/pyproject.toml` fixes the `extraPaths` half for everyone:
```toml
[tool.pyright]
extraPaths = [".", "uranium_tests/remap", "uranium_tests/remap_yaml", "uranium_tests/lib"]
```
`tests/` is already pyright's root (nearest ancestor with `pyproject.toml`), so it lands in the right scope with no per-developer configuration. Only the venv location stays machine-local, and developers who `uv sync` in-tree wouldn't need to set even that.
One caveat if you take this: a `pyrightconfig.json` overrides `[tool.pyright]` entirely, so it's worth mentioning in the docs which one wins.
## Suggestions that would make the pytest setup easier
### 1. Let a bare `pytest` invocation work
This is the biggest ergonomic win. Right now a front-end other than `urtest.sh` has to reproduce, exactly: `-p tools.uranium.plugin`, `--import-mode=importlib`, and a four-entry `PYTHONPATH`. Everything else already has an environment-variable fallback (`ATS_BIN`, `PROXY_VERIFIER_BIN`, `ATS_BUILD_ROOT`, `ATS_URTEST_SANDBOX` in `plugin.py:get_runtime`) — which is great, and is what made this tractable — but plugin registration and `sys.path` don't.
A rootdir `tests/conftest.py` would close the gap:
```python
import sys
from pathlib import Path
_TESTS = Path(__file__).parent
sys.path[:0] = [str(_TESTS), *(str(_TESTS / "uranium_tests" / name) for name in ("remap", "remap_yaml", "lib"))]
pytest_plugins = ["tools.uranium.plugin"]
```
`tests/` is the rootdir (inifile is `tests/pyproject.toml`), so `pytest_plugins` is legal there under pytest 8+. After that, any IDE test runner, `neotest`, or plain `pytest -k foo` collects and runs with only the four path options — no wrapper-specific knowledge.
### 2. Move `--import-mode=importlib` into the ini
```toml
[tool.pytest.ini_options]
addopts = "--import-mode=importlib"
```
One less flag every front-end has to know about, and it documents the requirement in the obvious place.
### 3. Don't depend on `uv run` for helper-process resolution
`microdns` and `microserver` are console scripts in the venv, exec'd by bare name — `replay.py:135`, `service_factory.py:104`, `service_factory.py:176`. That only works because `uv run` puts `.venv/bin` on `PATH`. Invoke the same pytest through any other launcher and it fails with:
```
FileNotFoundError: [Errno 2] No such file or directory: 'microdns'
```
which is a confusing failure to land on, since the package is plainly installed. Resolving them relative to the running interpreter would make the suite launcher-agnostic:
```python
shutil.which("microdns", path=str(Path(sys.executable).parent)) or "microdns"
```
### 4. `LD_LIBRARY_PATH` is a no-op on macOS
`runner.py:171` sets `LD_LIBRARY_PATH`, which dyld ignores; the darwin equivalent is `DYLD_LIBRARY_PATH` (or `DYLD_FALLBACK_LIBRARY_PATH`). Since `find_container_runtime` now prefers Apple container on darwin and `--no-run-in-container` makes native macOS runs a supported path, this is worth conditioning on `sys.platform`. It didn't bite me because the install carries usable rpaths, but it would for anyone whose build doesn't.
### 5. `tests/uv.lock` doesn't reach the build tree
`tests/CMakeLists.txt` does `configure_file(pyproject.toml pyproject.toml COPYONLY)` but has no rule for `uv.lock`, so `uv sync` in `<build>/tests` resolves fresh and writes its own lock. That means the committed lockfile never constrains the environment any run actually uses — including CI's.
It's already drifted. Comparing the committed lock against one generated in a build tree: same 53 packages, 8 at different versions.
```
charset-normalizer 3.5.0 -> 3.5.1
cryptography 50.0.0 -> 50.0.1
grpcio 1.83.0 -> 1.83.1
grpcio-tools 1.83.0 -> 1.83.1
gunicorn 26.0.0 -> 26.2.0
idna 3.18 -> 3.19
```
`cryptography` and `grpcio` moving underneath the TLS and gRPC tests is exactly the class of drift a lockfile is meant to prevent. Adding `uv.lock` to the `configure_file` set (and `--frozen` or `--locked` on the sync) would make build-tree environments reproducible.
## Minor doc note
`tests/urtest.sh` defaults to container execution, which is the right default, but the docs could state plainly that `--no-run-in-container` against a natively configured build works and is a reasonable macOS workflow — a large majority of tests pass there. The paths a front-end needs are all baked into the generated `<build>/tests/urtest.sh` as `--key=value` pairs, which turned out to be a very convenient integration point; worth keeping stable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] urtest: Convert AuTest tests to pytest [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13545:
URL: https://github.com/apache/trafficserver/pull/13545#issuecomment-6281376427
[approve ci autest 0]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org