Repository navigation
Conversation
One of them built RegisterRunner without the metadata field main added, so the station runner's tests stopped compiling. It, the snake_case finish status and the decoding of known and malformed event data all checked what serde writes or accepts, and they are gone. An unknown event still decodes to nothing, which the runner decides.
The crates' table tests move to rstest cases and fixtures, and the offline store checks its path names with relative-path instead of a hand-rolled check.
tracel-runtime declared its own JobDefinition with the same four fields as tracel_job::JobDefinition, and the process adapter only converted one into the other to build the command line and the definitions file. The crate now re-exports tracel-job's type, and the conversion is gone.
The tests ran a command line through run_launched, a copy of execute, so execute was never tested: that it writes the definitions before reading the command line, reads TRACEL_JOB_NUM from the environment, fails with exit code 1 when the definitions cannot be written, and succeeds after printing. execute now takes the command-line arguments and the environment lookup, Jobs::run gives it the process's own, and the tests call it directly. The copy is gone.
The Jobs::prepare tests sat in execution.rs and the duplicate-name test in the process adapter, away from the unit that decides them. They now live in jobs.rs, around one rstest fixture of two jobs. The prepare test no longer re-asserts the merge onto the default, which job.rs decides, only that the prepared job carries its name and the input it runs with. An unknown job and an input that does not decode are each refused with their own PrepareError variant, the unknown job with the registered names in order.
A panicked run's message is decided by catch_panic, and each thread seeing its own scope by the scope module, which both test them; the execution tests re-asserted them. They now check that a panic fails the run, and the two-thread test is gone. The scope test asserted the number and the input of two runs in one table; they are now two tests, the number one rstest case per execution, each reading back what the job saw.
The process tests re-asserted rules other units decide and test: the merge onto the default (job.rs), the panic message (panics.rs), the command line generated from the input schema (tracel-job), and the help and version text clap prints. They now assert what the adapter maps: a command line to the request it reads, the job and input to run or what to print, and each way a run does not complete to its exit code, one rstest case each. One test keeps that a job's schema reaches its flags. The jobs are one rstest fixture, a job that completes and one that fails, and record nothing: the tests compare the request a command line gives, and errors by variant and fields, never by message. Test names state the outcome.
The definitions test also read the file back through DefinitionsFile and compared it with its own to_json, a round trip tracel-job owns and tests. It now checks only the JSON describe writes. The failed-write test checks the path the error holds instead of looking for the file name in its message.
The test of an input without a default also checked what serde makes of null, for a struct and for (), and the prepared input test compared three prepared inputs, two of them with JSON written out by hand. The first now checks only that the input decodes as given, and the second that the prepared input is the JSON of what the job ran with. The inputs that do not decode are rstest cases that check only that the input is refused.
Two branches had no test: a scope entered over another restores it when its guard drops, and write_atomically removes its temporary file when the rename fails. The last test used to fail at the write instead; it now renames onto an existing, non-empty directory.
RunnerEventStream gave a frame that did not decode as an error, and serve_stream ended the session on any error, so one bad frame dropped the connection and failed every running job. The stream now skips such a frame with a warning and ends only when reading fails, which it logs; it yields plain events, and serve_stream has no error to handle. The stream is generic over BufRead, and its tests feed it bytes: a frame whose data does not decode is skipped, and a failed read ends it.
The input decodes in Jobs::prepare, outside the catch_panic that PreparedJob::run applies, so a panicking Deserialize unwound the job thread before it reported its outcome or left the in-flight count: wait_idle then blocked forever and the runner never reconnected. The executor now catches a panic around prepare too and reports it as the job's failure, and a dispatched job leaves the in-flight count through a drop guard, whether its thread ends, unwinds or never starts.
The sibling crates deny missing docs; tracel-runtime did not, and the variants and fields of RunnerError had none. The crate now denies them too, and RunnerError says what each variant means.
run only checked that the URL parsed, so one with another scheme, or one that cannot be a base such as `localhost:8000`, started the runner and then panicked joining the endpoint path. run now rejects a URL that is not http or https with RunnerError::NotHttp; any such URL can be a base, so the joins cannot fail. The HTTP clients are built in run too, and a client that cannot be built is RunnerError::HttpClient instead of a panic.
The serve tests threaded a recording Vec of job numbers through every test for the sake of one, waited on a Condvar with a ten-second timeout to make two jobs meet, and re-asserted the panic message and each thread's own scope, which panics.rs and scope.rs decide. The numbered test now serves a job of its own that records the number it is given, and the two concurrent jobs meet at a Barrier. How a job ended is one rstest case per outcome, comparing the statuses reported rather than the reasons, which are other units' error messages; one test compares a failed job's whole finish, with the job's own error as its reason. Test names state the outcome.
The protocol and SSE parser tests had given/when/then names, unlike the rest of the crate; they now state what each one shows.
The crate keeps its reasoning in names and rustdoc. The two comments left in serve.rs repeated what the warning and serve_forever's own rustdoc already say, that a rejected finish is expected after a disconnect and that running jobs drain before the runner reconnects.
The input schema is registered with the referencing crate, whose registry failed to prepare when the schema referred to another document or named a $schema it did not know, since its default retriever fetches nothing, and the job then had no flags at all. The registry now has a retriever that answers every such URI with the schema `true`, without fetching it, and the schema is registered under a hierarchical base URI, so a relative reference such as `shared.json` resolves too. Preparing then succeeds. A field whose reference still does not resolve, because it leads into a document that was not fetched, nowhere, or round in a loop, takes a JSON literal, and every other field keeps its flag.
A bool flag took zero or one value, so in `<job> --shuffle '{"epochs":2}'`
the JSON document was read as the flag's value and rejected as an
invalid bool. A bool flag now takes a value only as `--shuffle=false`,
which is how tracel-cli forwards flags, so an argument after a bare
`--shuffle` is the job's JSON input.
Job subcommands never have a --version flag: only the program's top-level command does, when it gives a version. Reserving the name only took the flag away from an input field named `version`. Only `help` and `config` are reserved now.
DefinitionsFile::new wrote tracel-job's own CARGO_PKG_VERSION as sdk_version, which is documented as the version of the Tracel SDK the program is built with. It now takes that version from its caller, and tracel-runtime passes its own.
The protocol states that `jobs` is ordered by name, but DefinitionsFile::new kept the order its caller gave. It now sorts them, so every file it writes keeps the stated order.
It was documented as an example that is typically the default input, but the command line always treats it as the default input: its values are the flags' defaults and the input is merged onto it. The docs now say so, as the crate docs already did.
A field's help fell back to the description of the type its $ref points to, but not through the anyOf of that type and null that schemars writes for an Option, so an undocumented `Option<Backend>` had no help. The fallback now follows the optional's value as well.
The command tests built jobs through a Definition fixture that duplicated JobDefinition and gave every job the same description, so the subcommand test could not tell whose description it saw. The fixtures now return JobDefinition with distinct descriptions, and the test checks each job's own. The flags tests renamed their Optimizer and Resume fixtures, which the command tests define differently under the same names. Each test module allowed dead code as a whole; the expectation now sits on the fixture types that need it. Two command tests are renamed after what they assert.
The command tests searched clap's rendered help and argument settings, matched clap's error kinds and compared ConfigError messages. They now check the input a command line gives: the field a flag sets and the value it reads, that a required flag may be left to the JSON input or the --config file, the order the layers win in, and the ConfigError variant and path. The merge rules are checked on overlay itself, and repeated cases are rstest cases.
Tests now show that an object that may be null takes a JSON literal, and that listed values leave out null, take JSON when one is not a scalar, and keep numbers as numbers.
local.rs ran to about a thousand lines doing six jobs. It is now local/ with a module each for run directories and numbering, the status file, the events line format, and artifacts, the session and provider staying in mod.rs. The events line format is the file contract other tools read, so it gets its own module and tests. A pure move: no behaviour changes, and each test moves to the module that owns what it checks.
save_artifact joined the name onto the run's artifacts directory unchecked and removed whatever was there, so `..` deleted the run and an absolute name deleted that path, since joining an absolute path replaces the base. The reader had the same gap. Both now read the name as a relative-path RelativePath and normalize it, refusing a name that resolves to the artifacts directory itself or leaves it, as `..` does. The directory is built with to_path, which ignores a leading `/`, so an absolute name stays inside the run. Nested names such as `checkpoints/best` stay allowed, as tracel-artifact's own bundle paths allow them.
The reader refused only empty ids and ids holding a slash or a backslash, so `.` and `..` named the experiment's own directory or the runs directory above it. Experiment names were not checked at all, so `..` or an absolute name put runs outside the runs directory and its .gitignore. Both are now read as a relative-path RelativePath that must be one plain component, and the directory is joined from that component, so a leading `/` cannot leave the parent. The store's docs say how both experiment and artifact names are read. The rule is tested on names, then once through each caller: create_experiment creates nothing for a name it refuses, and the reader loads nothing through an id that climbs out and back into a run.
The batch test asserted four counts at once, one of them len, which only adds the other three, and the rfc3339 and wire format tests checked what serde writes. The batch test now shows only that a metric descriptor is sent once per name.
The job number test asserted a number and a text in one test. Each is now an rstest case of the rule that a job number is recorded as a number when it is one.
The crate and scope docs said a thread has no job "in a plain binary or a unit test", and FinishSink said it was split out so the loop is testable with a fake. A public doc says what the code does: there is no job outside one, and the sink reports job outcomes to the station.
The panic test compared each failure with the exact message catch_panic words, all in one test. A panic now fails the run, a returned error is the run's failure as it was returned, and the panic message is the text the payload carries, a &str or a String, as rstest cases.
One test checked what the functions give both while a scope is entered and once its guard drops; they are now two tests, the first comparing with the scope itself rather than numbers written out again. The two threads entering their own scopes now meet at a Barrier while both are entered, so a scope shared between threads would fail the test, and the held scope is compared with the scope it holds.
The test of how a job ends ran three jobs and asserted six things about them, among them the failed job's exact message. A job that completes, fails or panics is now an rstest case of one fixture, each checking the status of its outcome; the error a job fails with is job.rs's to test.
The definition test spelled out the default as JSON and also restated that a bare function has no description or default; it now compares with the fixture's own default, and the bare case is gone. The merge test builds the expected input from the default it was merged onto, the failed job is checked by the error's type rather than its message, and the schema is compared with the input type's own. The repeated runs compare what each run was given instead of asserting in a loop, and the tests that only sent an input through serde and back are gone.
The two write tests each checked the contents and that no temporary file was left, the second by guessing the temporary file's name. The contents written and the replaced file are now one test each, and no temporary file is left is its own test, listing the directory as the failed-rename test does.
parse kept the one doc comment left among the process test helpers; its name and signature say what it reads.
The definitions test compared what describe writes with the file's JSON, which serde decides. The process adapter's tests already show that a launcher asking for the definitions gets them.
The test counted the finishes, then looked each one up in a loop. It now compares the statuses reported with both jobs completed, as the other serve tests do, and takes the setup from the fixture.
Two parser tests checked a frame field by field, one of them after counting the frames. They now compare the whole frame the parser gives.
runs_are_numbered_from_one_under_the_experiment_name also checked each run's directory, which the location test pins, so it now checks the numbers alone. The test of a runs directory that already exists takes its run from the local_run fixture.
the_status_follows_the_run_from_the_start_to_the_end pinned the file twice, checked its timestamps against chrono's format, and checked a temporary file that the rename never leaves. Keeping the start and stamping the end are now checked on the status itself, and the store writing a finished run as completed is tested with the session that writes it.
a_run_records_each_event_as_a_line_of_json drove one run through every kind of event and compared each line it read back, which is what serde writes, so it is gone. The attribute lines, which the session writes as the run starts, are pinned with the session, in order of their keys.
jwric
force-pushed
the
fix/tracel-runtime-review
branch
from
October 15, 2026 18:10
148c628 to
d4283a5
Compare
jwric
changed the title
The test made its own store and the first run in it: it now saves from the local_run fixture's run and loads from a later run of the same store. The one-file artifact type loses its comment.
The activity tests checked each ending in a test of its own and reached into events by position. Endings, of a guard and of a run, are now rstest cases compared as status and message, which ActivityStatus now derives PartialEq for. Progress updates are one rule tested with and without a meter, and each test asserts what it is named for. The tests of a guard failing with the panic that unwinds it and of running an activity, once in lib.rs and the tracing tests, move here beside the code that decides them; the ambient scope a run installs is checked through the current experiment, without tracing.
watched_run_logs_panics_from_other_threads lived in lib.rs, checked the panic hook's line format through the thread name, and asserted both the error log and the flush, the second under a comment. Two tests in panic_watch.rs now check one each: a watched run logs a panic on another thread as an error ending with its message, and flushes what it logged.
scoped_handle_inherits_attributes_and_call_site_wins asserted the level, the message, an inherited attribute, an overridden one and a third, two of them under comments. Which attribute wins is LogRecord::inherit_attrs's rule, now tested in log.rs, and the lib.rs test checks only that a log through a scoped handle inherits its attributes.
The run tests reached into events by position, matched panic reasons on the hook's format and thread name, and asserted several outcomes at once through a helper that returned each log's level, message, attribute count and activity. Each test now checks one outcome on the mock_run fixture: telemetry through an activity is one case per kind, a handle of a finished or dropped run is inactive and discards logs, a cancelled run ends as it is ended, and a stop request is logged once, as a message. run_context_records_events goes: every test that reads a log back shows it.
The tests built their fixture by hand with Fixture::new: it is now an rstest fixture. run_ends_the_experiment_by_what_its_code_returns is one case per outcome and checks only how the experiment ended, not that the output and the error pass through. The test that run runs nothing when the experiment does not start no longer matches the backend's message, and the current experiment test leaves the end of the scope to the context tests.
The context tests asserted the whole stack of scopes in one test, the current experiment before and after every await, and that none was left on the test thread, each behind an expect message. Each test now checks one rule through the current experiment's id: an inner scope overrides the outer one, leaving it restores the outer one, leaving the outer one first keeps the inner one, a handle enters its run on another thread, and instrumented tasks keep their own experiment across an await, as does one spawned in the current experiment.
The run control tests asserted both the run's token and an activity's linked one, and a registered activity's cancel both returned true and cancelled its token. Each outcome is now its own test, and whether cancel_activity finds an activity is one rule with a case registered and one unknown.
The test child guarded its own cancel, so it counted at most one cancel whatever the token did, and the concurrent test slept so its two cancels would race. The child now counts every cancel it is sent, and the two threads cancel together behind a barrier. The tests are named for the outcome, and the one restating that a cancelled token is cancelled goes.
The tracing tests lived beside the span helpers, matched each log's message, and checked a span field and an event field overriding it in one test under a comment. The layer's rules, routing to the current experiment or to one a span names, skipping events outside any, and turning event and span fields into attributes with the event's winning, are now tested in layer.rs, one rule each, counting the logs routed. The span helpers keep a test each, through the public tracing_span of a run and of an activity.
The progress tests read the started activities by position and asserted every name, parent, meter and attribute at once, and the endings test listed four statuses under a comment each. Each rule now has its own test, finding activities by the names the test gives them: how the training and evaluation activities nest, what their meters count, the epoch an epoch activity numbers, and how a split and the training end, one case per outcome.
A run carried a location, its console page or its offline directory, so that a job's experiment could print where it recorded as it started. The console page was guessed by taking `api/` off the API URL and appending the web app's routes, after asking the console whether the project's owner is a user or an organization, and the offline directory is one the user chose. Nothing read the line, and tracel run gives its own note when it falls back to recording offline. A run no longer says where its backend keeps it, and ExperimentLocation, ProjectScope::owner_kind and Experiments::on_start, which only the line used, are gone.
jwric
force-pushed
the
fix/tracel-runtime-review
branch
from
October 15, 2026 18:46
d4283a5 to
eac7fad
Compare
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.
Follow-up to #202, which merged before its review. CI didn't turn on the station, burn and schema features, so a broken station test went unnoticed. Lint, tests and docs now build them.
The offline store had a few real bugs: saving an artifact named
..deleted the whole run, an experiment name could escape the runs folder, a run whose events failed to write stayedrunning, andflushcould return before anything was on disk. Those are fixed, names are checked withrelative-path, andlocal.rsis split into smaller modules.In tracel-runtime and tracel-job, one bad Station event no longer ends the session, a panic while decoding a job's input fails that job instead of breaking the runner, a schema with an external
$refkeeps its flags, and a bool flag no longer eats the argument after it. A console or Station target connects once. Experiment runs no longer have a location or print a line when they start, since nothing used it.The tests of these crates are rewritten with rstest, one behaviour per test, and no serde tests.
Breaking:
Target::Console { project }andTarget::Station { station },DefinitionsFile::newtakes the SDK version first (tracel-cli's describe test will need it), andExperimentLocation,Experiments::on_start,ProjectScope::owner_kind,TargetError::MissingSettingand theAmbiguousartifact errors are gone.