Regression tests for swadm

[cfzimmerman]

"A network switch is only as good as its CLI"

- Probably someone important at some point

swadm is a great tool, and I would like it to continuously improve. While working on #145, I observed how easy (and tempting!) it is to make breaking changes. However, a quick search suggests breaking changes would not be well received by scripts and docs. Such changes may be needed someday, but that should be a careful decision and certainly not an accident.

This PR adds infrastructure for regression testing swadm changes in CI. These diffs focus on the tx eq settings I'll soon be modifying, but hopefully the structure is easy to extend as other swadm projects arise.

[taspelund]

It looks like this is pre-existing, but do you have any idea why this is mixed case?

[cfzimmerman]

This was new to me as well. Seems imposed by Cargo: https://doc.rust-lang.org/cargo/reference/environment-variables.html#:~:text=cleaned%20between%20builds.-,CARGO_BIN_EXE_,-%3Cname%3E%20%E2%80%94%20The

[taspelund]

You could always move sleep/timeout into args and expose a thin wrapper (fn or macro) that supplies the default. Not something I feel strongly about though, so feel free to ignore me if you disagree

[cfzimmerman]

Haha I often write comments trying to exonerate suspicious code only to realize later the comment could just be replaced by less suspicious code. Seems the case here. Thx!

retry now just calls a function retry_with with these defaults.

[taspelund]

This matches 0 or more characters, right? Is that worth calling out in the comment, since the others call out 1 being the minimum match length?

[taspelund]

this is slick, I didn't know you could do this inside of let destructuring

[taspelund]

This feels like it would be a good use case for expectorate

[cfzimmerman]

I agree that this particular case is a good use of expectorate. However since it's a test for the actual matcher, using that instead of expectorate is the whole point.

That begs whether expectorate is a better tool than expect_line. I'm putting a comment in the next commit, but my guess is not.

Tx eq is a good motivating case because it's arbitrarily structured and exposes uncontrolled state.

In this case, the values outside parentheses are what we have commanded, and afaik the values inside parentheses are what the hardware itself has decided to do. This occurs in the case of self optimizing tx eq. So we only want to assert the values outside parentheses controlled by swadm command. Hence the PARENS matches asserting that something exists there, but idk what's inside.

In my understanding, expectorate does exact matching, which makes it a poor fit for a fuzzy assert like this. Is that accurate?

[cfzimmerman]

Here's another plausible design:

• Get swadm output into a machine readable format. Either by adding formatted output options or building parsing infra.
• Allow tests to restructure that output to extract unnecessary tokens.
• Store known good versions of that output in files, and use expectorate for diffing.

However, unless someone feels strongly for a solution of that form, I'm disinclined because it seems like more required code per each new test.

[taspelund]

Similar feedback here about expectorate. As someone unfamiliar with the output of swadm link serdes get txeq, I have no idea what this test is looking for. I think being able to look at some output that's checked into get as a comparison would make it much simpler to reason about the test.

[cfzimmerman]

I'm not yet convinced about expectorate, but this is a good point. I'll add an EXAMPLE comment.

[taspelund]

Sure, no worries. I don't know expectorate super well, but I've seen it used for command output checking, both structured and unstructured, so I thought it would be worth evaluating. If you don't feel like it's a good fit, that's fine by me

[cfzimmerman]

Sorry for the repeated pings on this. Looks like we use expectorate in a lot of other crates. If I want people to contribute tests, probably good to use familiar tooling. I'll look for a way to make it work conveniently. Thx for suggesting.

[taspelund]

what are tap flags? I'm not sure what this test is doing without digging into swadm to figure out what the tap flags are. Maybe a reference to a command, source file, or type name would be helpful?

[bnaecker]

This is referring to the flags like --main or --pre1.

The term "tap" comes from signal processing, and refers to one shifted copy of the signal we're operating on. So --pre1 is the gain on a copy of the signal shifted backward in time by one sample. It's one of the gains in the filter we apply to the signal to try to improve quality.

[cfzimmerman]

I'm generally against unwarranted jargon. But tap seems pretty well standardized in tx eq literature, and it's a super convenient name 😅

[bnaecker]

Yeah, the term tap holds roughly the same position in signal processing as process does in software. If you're mucking with it, you know what it means :)

[taspelund]

Overall, the changes LGTM. I would say that the main bit of feedback I have is that it's hard to track what the tests are doing without already knowing that area of the code (what is a tap flag? what does the expected output look like?).

I'd like you to take a look at expectorate and see if that would be a good fit for some of these tests. If it doesn't make sense in this case, then maybe just a multi-line comment showing what swadm output the test is comparing against would be helpful.

[cfzimmerman]

🤖 CLAUDE REVIEW

Nice cleanup — replacing the ad-hoc port-link.rs / counters.rs tests with a shared
cmd harness plus expectorate golden files is a real improvement, and the README note
about swadm being a string-typed interface is well placed.

I ran the suite against a live dpd + tofino-model rig (illumos dpd, remote model) to
check the goldens rather than just reading them. All five --ignored tests pass, in both
orderings I tried, and tx_eq_exclusive + cmd::test::pattern_statics pass in the
illumos test job's non-ignored run. So the substance is sound. The findings below are
mostly about the harness making failures hard to read and one way it can silently
weaken itself.

The one I'd fix before merging

EXPECTORATE=overwrite inside retry bakes in transient output

cmd.rs:310 try_expectorate calls expectorate::eq_file(..).eval(..), which under
EXPECTORATE=overwrite writes the file and returns true on the first call
(feature_predicates.rs → assert_contents_impl(.., OverwriteMode::from_env(), ..)).
Every golden comparison in this PR is wrapped in cmd::retry, whose whole purpose is
that the first attempt is expected to observe pre-convergence state.

Demonstrated on the rig:

$ EXPECTORATE=overwrite DENDRITE_TEST_HOST='[::1]' \
      cargo test --test cli set_partial_taps -- --ignored
test tx_eq::set_partial_taps ... ok

$ cat swadm/tests/cli/expect/tx_eq_100g_link.txt
Port/Link  rear0/0
Speed      100G

The State Up line is gone. The test still passes, forever, having quietly stopped
checking that the link came up. Since regenerating goldens is exactly what someone will
do after an intentional swadm output change, this is a live trap.

Suggestion: have retry (or try_expectorate) refuse to overwrite while retrying — e.g.
retry on the raw command output until a cheap predicate holds, then run the expectorate
compare exactly once; or check OverwriteMode::from_env() and collapse to a single
attempt.

Failure output

A passing run prints a scary diff; a failing one prints ten

cmd.rs:315-316 prints the expectorate diff and then bail!("Match failed"). Wrapped in
retry, every normal run of the tx_eq tests emits:

@@ -1,3 +1,2 @@
 Port/Link  rear0/0
-State      Up
 Speed      100G

string doesn't match the contents of file: ".../tx_eq_100g_link.txt" see diffset above
                set EXPECTORATE=overwrite if these changes are intentional
test tx_eq::set_partial_taps ... ok

That's from a green run — the link simply wasn't up on the first poll (this happened on
every run I did). Conversely, a genuine mismatch prints ~10 stacked diffs and then fails
with the context-free Match failed, which doesn't even name the file. Worth putting the
file name in the bail! and only printing on the last attempt.

CI

packet-test-common.sh:105 — swadm tests no longer run if any packet test fails

errexit is on when the dpd-client suite runs (line 105), and the swadm block was moved
to the end of the script (lines 122-134). Before this PR the swadm checks ran first, so
both suites always reported. Now one flaky packet test aborts the script and every swadm
result disappears — the opposite of the "run all the swadm tests in CI" goal.

Moving swadm after the packet tests is right (these tests destroy rear0/0), so the fix
is to stop letting the first suite short-circuit the second:

set +o errexit
DENDRITE_TEST_HOST='[::1]' ... cargo test ... -- --ignored --skip succeeds_when_table_fragmented
packet_status=$?
set -o errexit
...swadm block...
exit $packet_status

tx_eq_exclusive never runs in the Linux job

It's the only CLI test without #[ignore], and the packet-test job passes -- --ignored,
which runs only ignored tests. So the one test covering clap's conflict output is
excluded from the job the new README points at; it actually runs in the illumos test
job (verified passing). Either mark it #[ignore] for consistency or add a second,
non---ignored invocation.

Smaller things

• cmd.rs:106 — TIMEOUT = 2s with a 100 ms sleep gives only ~5-10 real attempts once
  you account for a process spawn plus HTTP round trip per attempt. On my idle rig
  create_100g_link needed a retry every run to see State Up; on the CI box, with
  tofino-model and the packet tests' fallout still in flight, 2 s for delete → create →
  enable → link-up is thin. Consider retry_with and a larger budget for the link-up wait
  specifically.

• tx_eq.rs:71 — create_100g_link(port, link) hardcodes rear0/0 in the pattern (and
  in expect/tx_eq_100g_link.txt) instead of using its link argument, so calling the
  helper with any other link fails the compare rather than checking the link asked for.

• link_apply.rs:92 — "Ensure a consistent slate for link apply" isn't quite true for
  tx-eq. dpd's delete_link (dpd/src/link.rs:704) only sets delete_me and pokes the
  reconciler, and the software tx-eq values survive a delete/create: after
  link delete rear0/0, waiting for the 404, then link create rear0 -s 100g --fec rs
  with no tx-eq flags, link serdes get txeq still reported main -22 / post1 5 from the
  previous test. The tests pass only because both write paths (serdes set txeq and
  link apply) send all five taps. Fine today, but the comment oversells it, and any
  future test asserting a default tap value after create will be order-dependent.

• tx_eq.rs:59-66 vs link_apply.rs:93-107 — two different link-teardown helpers, one
  of which waits for the delete to land and one of which doesn't. The non-waiting one only
  works because create_link has an explicit "exists but marked delete_me → reset
  config" branch (dpd/src/link.rs:583). Worth collapsing into one helper in cmd.

• cmd.rs:69 / cmd.rs:79 — the doc tells the reader to use swadm_exact for args with
  spaces, but it's private, so sibling modules (tx_eq, link_apply) can't call it, and
  the intra-doc link points at a private item. Make it pub.

• cmd.rs:87 — "12224" duplicates common::DEFAULT_DPD_PORT; use the constant.

• swadm/Cargo.toml:30 — predicates = "3.1.4" is pinned in the crate while the other
  three dev-deps added here went through [workspace.dependencies]. Worth being
  consistent.

• Dropping syn / proc-macro2 from [workspace.dependencies] is safe — aal_macros
  pins its own versions and nothing used .workspace = true for either. Just noting I
  checked.

[cfzimmerman]

My responses:

1. I noticed as well that the link usually isn't up by the first retry. Nice motivation for the retry wrapper. I think it's okay to just look for this when reviewing swadm PRs. Version control makes the diffs visible anyway. Complicating the retry loop to avoid this seems unnecessary.
2. I again find this okay. If someone sees errors on a passing test and looks at the code, they'll find a retry loop. Seems self explanatory. Ideally we'd not emit expectorate errors during retries, but they go straight to stdout anyway.
3. Also okay with this. If the CI job is going to fail, might as well let it exit early. A PR should only merge on all greens anyway.
4. Good observation. I'll add non --ignored tests to this as well.
5. cmd timeout of two seconds seems alright for now. I'll look into it again if issues arise.
6. Now addressed. tx_eq and link_apply tests now use the same deletion helper, and it doesn't rely on the file constant. Made swadm_exact public, switched to DEFAULT_DPD_PORT, and moved around the dependency declaration.
7. The comments about link creation ordering rely too much on knowing the internals of DPD. And I have not verified these claims. The tests as-written treat DPD as a black box and seem sensible. I don't think they'll flake in CI. If tests fail upon iteration due to some unclear DPD design decision, that's a valid failure imo.