Conversation
bneradt
changed the title
bneradt
changed the title
1b8339e to
678cb46
Compare
bryancall
left a comment
bryancall
left a comment
There was a problem hiding this comment.
Leaving this as a comment rather than a formal review since it is still a draft. There is a lot of good work here, and two things that I think need to come out of this PR before it goes further.
First the good, because the framework itself is well built:
- Replay manifests are collected as first-class pytest items, so each scenario gets its own result and sandbox instead of hiding behind a thin registration wrapper.
- Validation parity with AuTest is preserved rather than quietly dropped.
diags.logis still scanned forERROR:,FATAL:and unrecognized configuration values, background processes are checked for premature exit, and_validate_goldreproduces the{}and backtick wildcard semantics of the old matcher. - Cross-worker port allocation uses a flock'd counter plus a bind probe, which is a genuine improvement over a per-process allocator.
- Sandbox paths are deliberately short and hashed with the rationale tied to
sockaddr_un.sun_path's 108-character limit written down, andprepare_sandboxrefuses tormtreeanything that is not a direct child of the sandbox root. - The consolidation is real rather than lossy, and the framework ships its own unit tests instead of being validated only by the suite it runs.
Two production changes are buried in the test migration
This is my main concern and it is a process point rather than a code-quality one. Copilot declined to analyze this PR because it exceeds its file limit, so these two changes have had no automated review either, and at 2414 files no human is going to find them by reading.
src/api/InkAPI.cc:8240 TSSslClientCertUpdate()'s lookup key changes from swoc::bwprint("{}:{}", cert_path, key_path) to key.assign(cert_path).
I traced this rather than assuming. The inner map is keyed by certificate path alone: SSLConfig.cc:952 sets ctx_key = client_cert, and the only producer of that key, SSLSNIConfig.cc:185, stores a fully resolved absolute path, which is what the cert_update plugin passes via traffic_ctl plugin msg. So on master the composed cert:key string can never match, the API always falls through and returns TS_ERROR without touching anything. After this change it locates the bucket, calls SSLCreateClientContext and swaps the shared context under ctxMapLock.
That is a dead-to-live transition on a public TSAPI affecting shared outbound TLS state. The fix looks correct to me. The problem is that its only covering test, cert_update.test.py, is deleted and rewritten in the same commit range, so a green run cannot distinguish "the API fix works" from "the rewritten test no longer asserts the same thing". This needs to land in its own PR against an unmodified test.
src/proxy/http/HttpTransact.cc:6399-6410 A new proxy.config.http.cache.max_stale_age_percent record, added end to end: RecordsConfig.cc:663, HttpConfig.h:708, OverridableConfigDefs.h:255, a new TS_CONFIG_HTTP_CACHE_MAX_STALE_AGE_PERCENT inserted before TS_CONFIG_LAST_ENTRY in apidefs.h.in:921, cripts/Configs.hpp:104, and the clamp logic in is_stale_cache_response_returnable(). None of it exists on the base and none of the seven commit messages or the PR body mention it.
In fairness: the default is 0 and the arithmetic collapses to the exact master expression at 0, so no behavior change ships. It is still a new proxy.config.* record and a new TSOverridableConfigKey, which is API surface the project supports forever, and it touches the stale-serving threshold used by negative revalidating and open_write_fail_action, which is load-shedding-critical.
One substantive note on the logic itself if it does get its own PR: get_max_age() returns 0, not -1, when a max-age directive is present but zero. So with the percent knob enabled, max_stale_age becomes 0 and any nonzero age fails the returnability test. An origin sending Cache-Control: max-age=0, which is common in front of revalidating origins, stops being served stale entirely during an outage. The documentation covers the absent case but not the present-and-zero case.
The Jenkins entry point is left in a non-working state
ci/jenkins/bin/autest.sh:123
The entire change to this file is one line, -D ./tests/gold_tests to -D ./tests/uranium_tests. Lines 107-108 still resolve AUTEST=/usr/bin/autest and line 123 still executes it. I counted the new tree: tests/uranium_tests has 0 files matching *.test.py, which is AuTest's only collection pattern, against 233 *.test.yaml and 303 test_*.py, and tests/gold_tests no longer exists. tests/pyproject.toml also drops the autest dependency.
So this script either runs a discovery pass over an inventory it cannot see and reports green having executed nothing, or fails for a reason unrelated to the change under test. ci/coverage and ci/regression were correctly updated to call tests/urtest.sh; this one was missed. A CI job that silently runs zero tests is worse than one that fails.
The deprecation story does not hold
CMakeLists.txt:164-165
The PR body says temporary deprecated aliases retain ci-fedora-autest, autest.sh, the old CMake options and targets. I checked each: option(ENABLE_AUTEST) and option(ENABLE_AUTEST_UDS) are replaced outright, the three gates now test ENABLE_URTEST only, and tests/CMakeLists.txt renames the autest, autest_no_install and autest-uds targets with no ALIAS or forwarding target. CMakePresets.json has only ci-fedora-urtest, and there is no tests/autest.sh. The only deprecation shims are AUTEST_SANDBOX, AUTEST_OPTIONS and PYTEST_OPTIONS, and those live inside if(ENABLE_URTEST), so they never fire for a caller passing the old option.
CMake treats an unconsumed -DENABLE_AUTEST=ON as an unused-variable notice, not an error, so an external pipeline configured that way configures successfully, silently skips add_subdirectory(tests), and produces no test target at all. A job using the old preset fails loudly instead, which is the better outcome. Either add real aliases or state plainly in the body that the cut-over is not backward compatible.
Framework items worth fixing while it is still a draft
tests/tools/uranium/runner.py:225is_official_test_container()requires an exactID == "fedora"andVERSION_ID == "44"match, andchoose_docker_mode()defaults to Docker when that fails. When the CI image is bumped to Fedora 45 in some unrelated PR, urtest inside that container stops recognizing it, defaults to Docker mode, finds no docker client, and every shard dies. The safe default when already inside any container is to run directly.tests/tools/uranium/replay.py:657-668_check_metricssleeps a fixed interval then reads each metric exactly once with no retry._check_filesin the same file already does this correctly with a 100ms deadline poll. 17 manifests usemetric_checks, and sleep-then-assert-once under 8-way xdist is precisely the flake pattern this migration is meant to leave behind.tests/tools/uranium/runtime.py:38class RuntimeError(ValueError)shadows the builtin for the whole module, whileprocess.py:29definesProcessError(RuntimeError)against the real builtin. Two unrelated hierarchies with the same spelling in one package.tests/tools/uranium/replay.py:749and:793read_text()with no existence check, so a missing diags or gold file surfaces as a raw traceback into framework internals rather than the intended message. Every sibling path in the same class already guards withif path.exists().
One claim I checked and am withdrawing before anyone chases it: I initially thought ReplayItem.runtest hardcoding is_exclusive=False broke serial_tests.txt for replay manifests. On re-reading it is latent rather than live, so it is worth tidying but it is not causing anything today.
c654ac4 to
c680be9
Compare
|
Thanks for the detailed review. I addressed the feedback and force-pushed the amended commit.
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. |
|
[approve ci autest] |
|
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. MethodologyTwo hosts, both 32 hardware threads, 30 GB RAM, Fedora, gcc 16.1.1, Proxy Verifier v3.1.3 on both sides (verified identical, checksum
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. PerformanceFull suite, same host, same binary:
The gap widens with worker count, which points at the mechanism:
AuTest's worker durations at The cause is that AuTest's load balancing never engages. Uranium's per-test distribution for reference: p50 1.13s, p95 9.17s, max 90.6s. Code coveragegcov plus gcovr 8.6, identical flags and exclusions on both sides, integration suite only:
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. MemoryPeak resident memory across the whole process tree:
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. FlakinessSeven AuTest runs and six Uranium runs, across all parallelism levels:
Two observations that only a repeated run surfaces. A flaky test was distorting the timing. AuTest at AuTest does not run a deterministic set of tests. Executed totals across runs were 586, 586, 586, 586, 586, 583, 581. At Test inventoryI mapped all 564 master Exactly one test has no successor: Everything else is verified consolidation, and the counts hold up: the 18 Other findings
OverallThe 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 Happy to share the raw logs, per-test timing data, or the gcovr HTML reports for either side. |
7fcbc9d to
f3d4785
Compare
Procedural Uranium tests need reusable process ownership and curl targeting that works for multiple ATS instances and Unix sockets. The test tooling layout and command line also made framework code difficult to distinguish from the ATS test inventory. This patch adds fixture-owned ATS factories and ATS-aware curl requests, including UDS support, and converts representative basic coverage to the procedural API. It moves the harness and its unit tests under tests/tools/uranium, updates imports, and lets pytest own selection, parallelism, and collection options while the wrapper handles Docker execution.
The remaining compatibility tests still depended on AuTest process orchestration, which prevented the Uranium suite from being fully native and made parallel execution unreliable. This removes the compatibility backend and converts each test to direct Proxy Verifier replay metadata or a native pytest scenario. It also hardens process startup, logging, reload timestamps, DNS, and timeout handling so the complete suite runs reliably with eight workers. The test guide and runner documentation now describe the replay-first workflow and pytest-native selection and parallelism.
The initial pytest migration left explicitly disabled scenarios as permanent skips and made native curl commands cumbersome to read and write. Repository guidance also retained obsolete AuTest conventions. This patch restores the opt-in scenarios behind a pytest manual marker and adds --run-manual for deliberate execution. It also changes Curl to parse one shell-style argument string, updates every call site, and documents parameter and timeout expectations. This adds collection and service regression coverage and refreshes the Uranium documentation to match the native pytest workflow.
Procedural Uranium support had accumulated unrelated process, client, and assertion behavior in one large module, making changes difficult to isolate while scenarios depended on its public import path. The converted log filename scenario could also read asynchronously written logs before every destination had flushed. This patch addresses this by moving focused implementations into a services package behind the existing tools.uranium.services facade and updating internal imports, documentation, and facade coverage. It also waits for each expected log record before asserting so parallel runs do not race ATS logging.
Replay processing crosses pytest collection, YAML validation, and runtime execution, but the framework directory did not explain those boundaries. Closely related modules were therefore difficult to distinguish. This patch addresses this by documenting the direct replay and procedural call flows, each module and supporting directory, and guidance for placing new framework code.
Split Uranium manifests force readers to move between orchestration metadata and Proxy Verifier traffic when reviewing a single scenario. This patch embeds each file-backed default replay into its collected test YAML and removes the redundant companion files. It also adds an inventory check to keep default replay traffic self-contained. Variant-specific and process-specific replays remain separate where they describe different traffic.
The migration still mixed replay layouts, retained stale documentation references, and left several framework and CI edge cases unresolved. This patch groups direct manifests under replay directories, hardens checks, corrects container and Jenkins execution, and drops unrelated code changes from the migration.
Several converted tests lost AuTest matching and staging semantics or relied on fixed timing. Rebasing also replayed an older migration change that silently removed a newly merged cache configuration feature. This patch restores the upstream cache feature, stages Cripts sources where ATS searches for them, preserves the HTTP/2 counter's numeric variability, and waits for STEK cluster convergence. It also updates the native-test inventory for the test added on master.
Microserver health checks could claim empty header-only lookup keys, making missing-origin responses depend on replay file load order. Uranium also retained successful sandboxes, causing Jenkins to copy gigabytes of irrelevant output when any test failed. This patch gives health checks distinct custom lookup-header values. It removes successful sandboxes after teardown while retaining failed sandboxes for diagnosis.
The Go client can complete before ATS has flushed all access-log entries. Waiting for two generic HTTP/3 entries therefore made the scenario inspect a partially written log and fail intermittently. This patch waits for the required large-POST entry before it checks both expected records. This preserves the assertions while removing the logging race.
The slow-post abort test can fail during ATS startup when the source checkout is mounted under directories that the dropped service user cannot traverse, even though the certificate files themselves are readable. This patch copies the certificate and key into the ATS runroot SSL directory so their permissions and path accessibility match the rest of the test configuration.
Parallel CI can delay access-log buffer flushing beyond ten seconds, and the rate-limit driver can unlink its FIFO before the background holder opens it. These races fail without exercising invalid ATS behavior. This patch allows a bounded 60-second log wait and retains the FIFO until the holder is released so both tests observe their intended state under a loaded VM.
CMCD prefetch validation could issue its cache-hit request before the asynchronous next-hop fill released its cache write lock. Slow CI hosts then observed a legitimate WL_MISS and failed intermittently. This patch replaces fixed sleeps with synchronization on the relevant next-hop transaction and cache write gauge. The hit assertions remain unchanged while the setup no longer races the asynchronous fill.
Augmented assignment made process-output expectations easy to mistake for ordinary assignment. A typo could discard prior checks and let a test lose coverage without an immediate error. This patch introduces read-only stream expectation objects with explicit methods for regex, gold-file, reset, and return-code behavior. It also preserves captured output through clearly named text properties and adds focused misuse and validation coverage.
New AuTests landed on master while the pytest migration was under review, so rebasing without porting them would silently drop their coverage. Recent per-server metric changes also require derived-stat synchronization to avoid racing assertions. This patch converts replay-compatible coverage into combined manifests and extends the native connection-limit scenario for aggregate, hidden, and remap-overridden metrics. It polls derived metrics after shortening the sync interval, preserving the original behavior without fixed timing races.
A new AuTest landed while the pytest migration was under review, so leaving it in the old suite would silently drop its API coverage. This patch converts the custom-listener test to a native Uranium scenario. It uses a Python socket client and verifies the plugin accept callback through its diagnostics.
Uranium's source launcher assumed Docker and relied on Docker-specific markers. Developers using Podman or Apple container could not start the test image, and markerless Apple containers attempted an unavailable nested Docker launch. This patch selects Apple container on macOS and Podman on Linux, with Docker as a fallback. It introduces runtime-neutral flags while preserving the Docker aliases, marks managed launches explicitly, and recognizes Fedora 44 for manually entered Apple containers.
Preserve the coverage added on master while keeping the rebased branch free of AuTest files. Fold replay-friendly cases into existing YAML and use native scenarios for tests requiring custom clients or process control.
Make bare pytest and editor discovery work from the source tree, keep the Python environment locked, and resolve helper scripts from that environment. Support the macOS loader path and document the native workflow.
Wait for live verifier output before inspecting multiplexer copies, and flush HTTP access logs promptly. This removes races exposed by the full parallel test run without weakening the original assertions.
Connection coalescing could assign an origin socket to a queued transaction that did not own its connection-tracker reservation. The live connection was then absent from per-server limits and metrics. This patch keeps the reservation with the in-progress connection and transfers it to the established session. Failed connection attempts release the reservation from the coalescing entry. Fixes: apache#13605
Master added and updated AuTests while this branch was converting the suite to pytest. Leaving those files behind would silently drop their coverage once Uranium becomes the only runner. This patch ports replay-compatible cases to self-contained YAML and rewrites custom-client cases as native scenarios. It also carries forward the accompanying assertions and hardens asynchronous checks exposed by eight-worker runs. Co-authored-by: Codex gpt-5.6-sol xhigh
Worker-number directories made retained Uranium artifacts difficult to navigate because a test owner was not visible at the sandbox root. Failures from parallel runs therefore required extra lookup work. This patch names each item directory from its pytest identity plus a stable digest and places it directly under the shared root. The names remain short enough for ATS Unix sockets, while shared locks and port allocation preserve xdist safety. Co-authored-by: Codex gpt-5.6-sol xhigh
Shortened sandbox names and generated suffixes made test artifacts hard to identify, while automatic cleanup prevented inspecting passing runs. This patch uses full test names and clears named directories on rerun. It adds optional retention of passing sandboxes and rejects colliding names. Unix socket paths stay short independently of artifact paths. Co-authored-by: Codex gpt-6-astra medium
Changes on master conflicted with the Uranium conversion and left its origin metric checks behind the current publication behavior. This patch preserves upstream compression and metric-retraction coverage in Uranium after rebasing. It updates aggregate modes and metric names, waits for runtime publication changes, and updates process-lifetime guidance for fixture-owned services. Co-authored-by: Codex gpt-6-astra medium
6d69ecb to
57c0465
Compare
|
@bryancall Rebased onto current master, The rebase carried over the updated per-server metric coverage from #13666: the 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 The substantive conversion delta from the previous pushed head is in |
bryancall
left a comment
bryancall
left a comment
There was a problem hiding this comment.
❌ Requesting changes on the delta, this time on substance rather than on the branch state.
Thank you for the quick rebase. What follows is a review of the four commits it brought in (6f64c554, a71ce8fb, e438534a, 57c04650), read as one 49 file diff against 33f8edae, with each converted test compared assertion by assertion against its pre-conversion original. The three items I cleared on 2026-09-14 are untouched by this delta, which changes nothing under src/, plugins/ or the Jenkins entry point.
One housekeeping note, not a blocker and not what this review is about: the branch has drifted a commit behind master since the rebase and is showing conflicts again. That is ordinary churn, it does not affect anything below, and I am not going to keep blocking on branch state after you did the rebase I asked for. It will need one more touch before it can merge.
The headline: the conversion is mostly faithful, and in several places it is a genuine improvement. The problems are concentrated in one shape, a check that no longer has anything pinning when it runs, so it can be satisfied by a prefix of the evidence instead of the whole of it. That shape accounts for two of the four blocking items below.
Blocking
1. StillRunningAfter has no equivalent in the new framework, so five converted tests no longer notice that ATS died.
Every old test carried tr.StillRunningAfter = self._ts, which failed the run if the proxy was gone at the end. Nothing in the new harness replaces it. ATS.close() (services/ats.py:627-638) calls stop() and then greps diags for FATAL:; ManagedProcess.stop() (process.py:214-231) polls and discards an already-set exit status without validating it; the ats_factory fixture (plugin.py:309-316) adds nothing. ATS.is_running exists at services/ats.py:314 and no converted test calls it.
Most converted tests are incidentally covered, because they end on a traffic_ctl or curl call that needs a live ATS. Five do not, because they end on a file read:
tests/uranium_tests/pluginTest/abuse_shield/test_abuse_shield.py:241,:289-290,:340-341,:419-420tests/uranium_tests/pluginTest/background_fetch/test_background_fetch.py:140-153
Concrete case: background_fetch segfaults ATS in the replay continuation just after it emits Starting background fetch, replaying: for above-threshold. wait_for_file_lines has already matched the marker, the excludes at :152-153 then pass over a truncated log, and the test is green on a proxy that crashed. The old test failed. A SIGSEGV writes no FATAL: line, and neither does an ASan report.
Related, and the reason this is silent rather than loud: that FATAL: check is guarded by if self._was_started and self.diags_log.exists(). A missing log is treated as "nothing to check" rather than "the evidence I was supposed to read is gone", which is the wrong default for a harness.
2. "Must not appear" checks now run against a snapshot taken the instant the positive marker arrives, rather than against the settled state.
AuTest evaluated Testers.ExcludesExpression over the complete file after the run finished. wait_for_file_lines returns as soon as the positive pattern has one match, and the excludes then run on that prefix. wait_for_status (config_reload_helpers.py:76-85) has the same property: it returns the first snapshot where all contains are present and all excludes absent.
Five sites:
test_background_fetch.py:152-153(content captured at:140)test_abuse_shield.py:290,:341,:213test_config_reload_ssl_bulk.py:117-124and:142-147
The ssl_bulk pair is the clearest. excludes=("FAIL",) has nothing pinning terminality, and "success" cannot supply it because CtrlPrinters.cc:489 always prints an N success counter regardless of outcome. Concrete case: the coordinator's children complete out of order, SSLCertificateConfig logs ssl_multicert.yaml finished loading for the 20 certs and SNIConfig fails afterwards. The poll returns on the snapshot taken before the ✗ FAIL annotation is printed and the test passes. The old test read the tree at DelayStart = 10, after the reload settled, and failed.
The fix already exists in this PR: test_config_reload_ssl_state.py:84 uses excludes=("in_progress", ...), which is meaningful because CtrlPrinters.cc:481 prints Reload [in_progress] while the reload is live. Applying the same pin to ssl_bulk closes it. For background_fetch and abuse_shield, the equivalent is to evaluate the excludes after teardown rather than at first match.
3. Three excludes: in the new rate_limit replay are silently ignored.
tests/uranium_tests/pluginTest/rate_limit/rate_limit_metric_names.test.yaml:35, :37, :39 use excludes: inside file_checks. _check_files (replay.py:703-747) consumes exactly glob, path, exists, timeout, contains, line_count_min and matches. There is no excludes branch, and no schema validation, so the key is dropped and each of those three entries degenerates to "metrics.txt exists".
That matters here specifically. The test's description is "Verify rate_limit metrics use prefix.type.tag names". The three contains lines check the correct order appears; the three excludes lines were the half checking the wrong order does not. As written, a bug emitting both myprefix.sni.mytag.queued and mytag.sni.myprefix.queued passes. For a naming order test the negative half is the point.
Scope check: exactly 3 entries in 1 file. The other 55 occurrences of excludes in these YAML files sit in _validate_text contexts (replay.py:809) where the key is handled, so this is not fleet wide.
The root cause is worth more than the symptom. file_checks silently accepts unknown keys, so the next typo is another assertion that cannot fail. A strict key check would have caught this at authoring time.
4. Plugin initialized with 1000 slots per tracker, N rules is dropped for 8 of 9 abuse_shield instances, which makes two negative-only tests vacuous.
Kept only at test_abuse_shield.py:194 (the message test). Missing for the instances built at :228, :253, :274, :277, :303, :329, :354, :384, :409, :435. The old test asserted the exact rule count for every instance (old :336, :465, :591, :698, :820, :928, :1080, :1203, :1297).
Concrete case: a Config::parse regression that truncates the rules: sequence, for instance a break instead of continue in the loop at plugins/experimental/abuse_shield/config.cc:309-380, so only the last rule loads. test_abuse_shield_multiple_rules at :319 then asserts "strict_limit matched" (:340) and "lenient_limit NOT matched" (:341). With lenient_limit never loaded both pass, and the test proves nothing about token debt inheritance. Same for test_abuse_shield_rate_limited_ips at :264 if ordinary_req is the rule that is dropped (:290). The old rule count check failed in both cases.
Worth fixing, not blocking
The next three items are one trade, not three complaints. My first review called out the short hashed sandbox names and the written down sun_path rationale as a strength. Make Uranium sandboxes identifiable and Keep readable Uranium test sandboxes deliberately undo that, and readable names are a reasonable thing to want when you are staring at a failed run. What went with the digest and the length cap, though, was uniqueness and the length bound, so these three are the cost of that exchange rather than an objection to it.
5. item_sandbox and procedural_sandbox are now the same function, and uranium_replay deletes the tree it is handed.
runtime.py:151-166, both bodies are now identical:
def item_sandbox(self, replay_path: Path, node_name: str) -> Path:
return self.sandbox_root / self.sandbox_name(node_name)
def procedural_sandbox(self, node_name: str) -> Path:
return self.sandbox_root / self.sandbox_name(node_name)Before this delta they were disjoint by construction, <stem>-<sha> keyed on f"{relative}:{node_name}" against p-<sha>. Now the uranium_replay fixture (plugin.py:280-288) builds ReplayTest(spec, runtime, request.node.nodeid), whose self.sandbox = runtime.item_sandbox(...) at replay.py:58 resolves to the procedural test's own run directory, and ReplayTest.run() calls prepare_sandbox at replay.py:90, which is shutil.rmtree followed by mkdir (runtime.py:179-187). A test requesting both uranium_replay and any sandbox owning fixture (ats, curl, services, procedural_context) erases its own live process tree mid test. A multi variant replay run through that fixture wipes each variant's artifacts as the next starts.
The collection guard cannot catch this, structurally: it is keyed per item, and this is one item with two consumers of one name.
Latent today, and I want to be accurate about that: uranium_replay has no callers in tests/, doc/ or .github/. It is public, documented API though, and the first caller pays. The unused replay_path parameter is the tell that the two were meant to differ. Worth fixing while the commits that touch this are still in flight, either by feeding replay_path back into the name or by giving the fixture a subdirectory under the context's run directory.
6. The collision guard, which is now the whole safety net for the naming scheme, is unreadable in the mode the docs tell people to use.
The guard at plugin.py:90-100 is good work and I am not asking for it to be removed. Its placement is right: it runs over the full item list before -k deselection and before the --urtest-shard-* split, so a shard cannot miss a pair that landed in another shard. Enumerated statically over the tree, 604 replay names plus procedural function names, there are zero collisions today.
The problem is that it raises pytest.UsageError from inside pytest_collection_modifyitems, and under xdist that hook runs in the workers rather than the controller. Reproduced on pytest 9.1.1 with pytest-xdist: serially the message prints cleanly as ERROR: Uranium sandbox name 'x' is shared by ...; with -n 2 the run ends in
INTERNALERROR> assert not crashitem, (crashitem, node)
INTERNALERROR> AssertionError: ('ue/test_a.py::test_x', <WorkerController gw1>)
no tests ran
and the sentence "Give these tests distinct names" appears zero times in the output. pytest.exit(..., returncode=4) behaves the same. Both tests/README.md and doc/developer-guide/testing/uranium-tests.en.rst, as amended in this diff, tell developers to pass -n, and CI shards run that way. So the single safeguard for the new naming scheme reports a collision as a pytest internal error.
A print(message, file=sys.stderr, flush=True) before the raise is enough, verified: worker stderr is forwarded to the controller terminal and the message showed up under -n 2. Attaching the failure to the offending items would be cleaner.
7. The composed sun_path invariant was removed rather than made obsolete.
test_procedural_sandbox_leaves_room_for_ats_rpc_socket asserted that a composed sandbox path, sandbox / <process-name> / runtime/<socket>, stayed under the sun_path limit. Its replacement, test_long_sandbox_uses_short_uds_path at tests/tools/uranium/tests/test_services.py:158-172, bounds only ATS.uds_path. Meanwhile runtime.py:170-178 removed both the 10 character digest and the 48 character stem cap, so a sandbox directory component is now the unbounded full node label, class plus function plus parametrize id joined by __.
It is tempting to read this as obsolete, because the JSON-RPC socket already lives in a /tmp mkdtemp via runroot.yaml: runtimedir (replay.py:460, :475-477), and today's only in sandbox socket, ats.sock, is covered by the new >= 104 fallback at services/ats.py:193-201, which is the right bound (macOS sun_path is 104 bytes including the NUL, Linux 108). Both of those are true. Neither covers the fact that the invariant itself is gone. A test named TestPerServerConnectionAggregate::test_metric_aggregate_retraction_with_hostname_match[both-sum] yields a roughly 95 character component, and any future path limited artifact under that sandbox other than ats.sock overruns with nothing guarding it. Please keep a bound on the composed path, or restore a cap on the name.
8. Two concurrent runs on the same checkout now share a sandbox root. get_runtime used to append os.environ.get("PYTEST_XDIST_WORKER", "main"), so a -n 8 run lived under <root>/gw0..gw7/ and a serial run under <root>/main/. That component is gone, and runner.py:93 defaults the root to _short_sandbox(source_root), which runner.py:430-434 defines as /tmp/ats-urtest-<sha256(source_root)[:8]>: deterministic per checkout, no pid, timestamp or mkdtemp. Two serial runs already collided before this delta, so the regression is narrower than it first looks: what is new is that a parallel run and any other run are no longer separated. Starting urtest.sh -k test_basic while urtest.sh -n 8 is running is an ordinary thing to do, and the second run's prepare_sandbox rmtrees the first run's live directory. Success teardown at plugin.py:176 uses ignore_errors=True, so a partial failure to delete another run's tree is invisible.
To be fair to the guard at runtime.py:183-184, it is structurally fine: sandbox_name cannot emit a separator, ., .., or the counter and lock file names, so it does bound the deletion target. What it no longer establishes is that nothing else is using the directory. An exclusive lock on the root for the duration of a run, failing fast when a second run cannot take it, would settle this.
9. time.sleep(3) replaces DelayStart = 8 for the record triggered reload. test_config_reload_plugin_api.py:112-114, one assertion with no retry. This produces false failures under load rather than false passes, so it is not blocking, but it is the wall clock pattern the rest of this conversion avoids. Polling config status -c all for cfg_plugin_test with a deadline would match the style used everywhere else here.
10. The RPC error check is relaxed for the rpc-greet scenario. test_config_reload_plugin_api.py:77-78. Old validate_rpc_greet failed on any result.errors; the shared reload() now rejects only 6010/6011, and applies that to all four scenarios. If cfg_plugin_test is registered without a handler the server returns CONFIG_NO_HANDLER (6012) with no task created, and the RPC assertion passes. The failure signal survives, because wait_for_status("rpc-greet", ...) then fails on a missing token, but it arrives 20 seconds later pointing at the wrong thing.
11. The rule match log format is no longer pinned. test_abuse_shield.py:488 was Rule "(connection|request)_log" matched for IP=127.0.0.1 actions=\[log\] (old :1438) and is now matched.*actions=\[log\]. An address normalization regression emitting IP=::ffff:127.0.0.1, or dropping the field, passes now and failed before. That log line is the only output this [log]-only rule produces, so its format is the behaviour under test. The for IP= literal is dropped at :239, :241, :259, :289, :314, :340, :363, :394, :419, :462 too, though those lose less, since the old IP=.*actions already accepted an empty address.
12. test_slice_stale_generation.py:199-203 turns one required diagnostic into an alternation, PARSE_INCOMPLETE or Content-Length body underrun for key mixed. A regression returning a complete, parseable response whose body is short of the advertised length now satisfies the first branch. Lines :204-205 still pin the Mismatch/Bad block Content-Range diagnostic with specific blk_range and etag_got, which is why this is low.
13. test_runroot.py:180-181 recursively chmods the runroot to 0777/0666 before asserting PASSED. traffic_layout verify checks r/w/x for the configured admin user over the files inside each layout directory (src/traffic_layout/engine.cc:587-640). An init regression writing a file under var/trafficserver as 0600 owned by another user printed FAILED before and prints PASSED now. The pre-existing directory.chmod(0o777) already made this accommodation one level up, so this extends it rather than introducing it.
14. Port counter, three small things. The locking is correct, and I checked that the sandbox_root.parent to sandbox_root move is genuinely equivalent given the root change: same .port-counter and .execution-lock, same cross process flock, and the a+ plus seek(0)/truncate() pattern is right despite looking wrong, because the truncate moves EOF before the append mode write. At runtime.py:112-136: int(content) if content else 10000 collapses first use, an emptied file and a counter deleted mid run into one silent restart at 10001, which matters because ports are reserved long before they are bound; int(content) on a corrupt file raises a ValueError naming neither the file nor its contents; and except OSError: continue swallows EADDRNOTAVAIL and ENOBUFS identically to EADDRINUSE, after which the failure message names neither the errno nor the range.
15. Cosmetic. disable_log_checks=True at test_config_reload_ssl_bulk.py:44 and test_config_reload_ssl_state.py:41 is a no-op on this path: it is only read by replay.py:775 inside _validate_ats_logs, which runs only from the replay path. Intent matches the old files' comments, the flag just does nothing here.
Not yours, flagging so it is not lost
tests/uranium_tests/pluginTest/abuse_shield/idle_connections.py:32-40 swallows OSError per connection and returns 0 if connections else 1, so opening 1 of 30 requested idle connections exits success and the abuse shield threshold never trips. I diffed it against the pre-conversion file: moved byte for byte from gold_tests/, so it is inherited rather than introduced here, and I would not block a conversion on it. Worth its own issue, since converted tests now depend on it.
What I checked and found sound
The rebase is genuine: master 281b608361 is an ancestor of the head. add_autest_plugin to add_urtest_plugin in tests/uranium_tests/jsonrpc/plugins/CMakeLists.txt fixes three calls to a command defined nowhere in the tree, which was a configure time break; ADD_URTEST_PLUGIN at tests/CMakeLists.txt:20-37 emits to the .libs path the new copy_custom_plugin calls resolve to, and replay.py:528-529 raises on a missing file, so a path mistake fails loudly.
On the conversions themselves: no gold file comparison was dropped anywhere in scope, because no gold files existed in any of the old directories. No metric assertion was dropped, all 24 old traffic_ctl metric get expectations have counterparts. No return code check was dropped, and the jsonrpc set gains seven, since the old files pinned ReturnCode on only 6 of 13 CLI cases and the new ones pin all 13. Four of the six jsonrpc files are preserved outright, config_reload_ssl_state.py included, which is the file that already gets the terminality pin right.
Several changes are real improvements rather than neutral ports. test_per_server_connection_max.py corrects the metric spelling to per_server.current_connection.max. (ConnectionTracker.cc:512), which means the old negative assertion was vacuous and the positive ones unsatisfiable, and the metric_aggregate 2 to 3 change is likewise a correction (AGGREGATE_MAX suppresses the sums the test waits for). sni_queue_scenario.py passes the fourth argument that rate_limit_sni_queue_client.sh reads under set -u, so that test could not previously have run at all. test_rate_limit_sni.py adds a wait_for_file_lines on Queued VC is too old, replacing a check that would have passed on code that never expired a queued VC. The jax_fingerprint replay adds a backreference rule requiring the JA3 and JA4 vconn user_arg indices to be the same, which is the substance of shared slot reuse and was not verified before. h2_session_errors.py adds shutdown(SHUT_WR) and a drain, closing a case where close() with unread frames could RST away the GOAWAY under test.
Framework side: every wait helper raises on timeout rather than returning a default, wait_for_status is a bounded poll that raises with the last output rather than a blind sleep, and ATSFactory.close() aggregates failures into an ExceptionGroup instead of losing all but the first. That last one is rare enough to be worth saying out loud. file_checks: contains is re.search rather than a substring test (replay.py:738), so the regex bearing contains in the jax_fingerprint replay is valid. yapf and pyflakes are clean over all 34 changed Python files. The proxy.config.log.max_secs_per_buffer: 1 additions are a real record (RecordsConfig.cc:1081) and only make buffer flushing prompt enough for the existing checks to be deterministic.
This replaces Traffic Server's AuTest end-to-end suite with Uranium, a pytest
test framework for Proxy Verifier replays and native Python scenarios. There is
no AuTest compatibility backend in the resulting test suite.
Summary
<scenario>.test.yaml; eachmanifest contains both its
urtestconfiguration and replay sessionspytest scenario classes with an explicit
run()entry pointand shared/exclusive scenarios
under
tests/tools/uraniumurtest.sh, Uranium CMakeoptions and targets, and
urtestreplay metadataDirect replay manifests live in an existing
replay/orreplays/directorywhen a test tree has one. The
at_headersmanifest intentionally remains nextto its plugin assets.
Developer workflow
Run selected tests with pytest's normal selection and parallelism options:
A configured tree generates
<build>/tests/urtest.shfor running against itsinstalled ATS tree. Manual tests are skipped by default and can be selected
explicitly with
--run-manual -k <expression>.Container behavior
The source-tree runner defaults to
ci.trafficserver.apache.org/ats/fedora:44, performs an incremental dedicatedbuild/install, and runs pytest there. It avoids nested Docker whenever it is
already inside any container.
--run-in-dockerand--no-run-in-dockeroverride that choice.
The official Fedora image also enables the optional
cdifflibacceleration forlarge gold-file comparisons. Other environments use it when installed and
fall back to the standard-library
difflibimplementation otherwise.Compatibility
This is an intentional cutover rather than a deprecation shim. The AuTest
runner, backend, CMake targets, presets, and options are removed. CMake reports
a fatal migration message if removed
ENABLE_AUTEST,ENABLE_AUTEST_UDS,AUTEST_SANDBOX,AUTEST_OPTIONS, orPYTEST_OPTIONSvariables are supplied.Existing CI scripts now invoke Uranium and pass pytest's
-kand-noptions.SHARDandSHARDCNTcontinue to distribute pytest items across CI shards.Validation
failure for the pre-existing
TSSslClientCertUpdatedefectchecks passed