Skip to content

Commit a2b4f83

Browse files
authored
Refactor Go tests to use Testify (#160)
## Description Use testify to shorten test assertions and make them (subjectively) easier to read. It also updates the test names to include more intent as per our updated test skill. ## Release intent <!-- Keep the fences exactly as they are; CI parses between them. --> <!-- Set each bump to none, patch, minor, or major. Leave the key names. --> <!-- `services` must be >= the highest component bump. --> <!-- Set the title and body to n/a when no bump is a release. --> <!-- The release version users see is NOT declared here: it patch-bumps by itself whenever any bump above is a release. --> <!-- Keep the changelog body to prose and bullets. A line starting with "### " ends the section and silently truncates the rest. --> <!-- pair-release-intent:v1 --> ### Changelog title n/a ### Changelog body n/a ### Bumps - services: none - nvpair-cluster-manager: none - nvpair-engine-manager: none - nvpair-errors: none - nvpair-job-scheduler: none - nvpair-manual-nodes: none - nvpair-node-info: none - nvpair-node-scanner: none - nvpair-node-settings: none - nvpair-proxy: none - nvpair-tui: none - nvpair-ui-broker: none - nvpair-workload-manager: none <!-- /pair-release-intent:v1 --> ## Scope This does not affect production code at all. It adds a mature, commonly used assertion library and renames tests. ## Validation CI passes. Everything compiles locally and gofmt passes. Note to reviewers: Look at SKILL.md and then spot check some files. It's not expected to read through all changes. ## Risk Some assertions are very slightly different in semantics: - FileExists uses os.Lstat, whereas the old checks used os.Stat. Directories now fail, but dangling symlinks can pass. - Empty checks intentionally accept either nil or empty slices. However, Equal still distinguishes them in some tests. - NaN rejection: The scheduler’s InDelta assertions (line 73) now reject NaN. The original tolerance predicate accidentally accepted it. Otherwise, every attempt was made for this to be a mechanical change. ## Checklist - [X] I have read the [Contributing Guidelines](https://github.com/NVIDIA/Personal-AI-Router/blob/main/CONTRIBUTING.md). - [X] Every commit is signed off (`git commit -s`), certifying the [Developer Certificate of Origin](https://developercertificate.org/). - [X] New or existing tests cover the change. - [X] Relevant documentation is updated. - [X] I checked the diff, changed filenames, and commit messages for credentials, private data, internal URLs, internal issue identifiers, and generated artifacts. - [X] I recorded the validation commands and results above. - [X] I declared version bumps in the release-intent block above. `services/versions.json` is written by automation — do not edit it by hand. --------- Signed-off-by: Kaylee Lubick <klubick@nvidia.com> Signed-off-by: Terve <ntervalon@nvidia.com>
1 parent 735d1e6 commit a2b4f83

287 files changed

Lines changed: 10985 additions & 20643 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Lines changed: 106 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
---
22
name: pair-test-style
33
description: >-
4-
Write, refactor, or review tests in Personal AI Router using focused cases,
4+
Write, refactor, or review tests in PAIR using focused cases,
55
descriptive subtests, explicit error handling, and structured assertions.
66
Use when adding or changing Go service or desktop tests; apply to the tests
77
in scope rather than starting a repository-wide cleanup.
@@ -11,7 +11,7 @@ SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All
1111
SPDX-License-Identifier: Apache-2.0
1212
-->
1313

14-
# PAIR test style
14+
# NVIDIA PAIR test style
1515

1616
Keep each test's setup, action, and expected behavior easy to follow. These
1717
conventions apply to contributors and coding agents working on repository tests.
@@ -21,44 +21,86 @@ They guide test structure; they do not require new coverage or unrelated rewrite
2121

2222
- Give each top-level test one behavior to prove. Split long tests that combine
2323
validation, persistence, restart, and recovery into independently readable tests.
24+
- Use as many assertions as needed to prove that behavior. Keep assertions
25+
focused on the relevant results or fields; check unrelated behavior in a
26+
separate test so failures have a clear meaning.
27+
- Exercise the interface callers use and assert observable results. Internal
28+
refactoring that preserves behavior should not require rewriting tests to
29+
match private helpers, internal state, or incidental call order.
2430
- Keep a table-driven test dedicated to its related cases. Put an unrelated
2531
one-off assertion or scenario in its own test function.
2632
- Prefer explicit cases over nested loops and boolean mode matrices when the
2733
combinations obscure what is being checked. Keep a matrix when the combinations
2834
themselves are the behavior under test and the names explain them.
29-
- Name subtests after their scenario or outcome: `process mode`, `command mode`,
30-
`rejects bind override`, or `preserves response body`, rather than `true`,
31-
`false`, or an index.
35+
36+
## Name the behavior being proved
37+
38+
- Name Go tests `Test<Subject>_<ExpectedBehavior>`, for example
39+
`TestParseAddress_RejectsMissingPort`. Include the condition when needed to
40+
distinguish the behavior, as in `TestScheduler_SkipsNodesWhenEngineIsStopped`.
41+
- Consider three candidate names before selecting the one that most clearly
42+
states the subject and expected behavior. Prefer concrete outcomes over vague
43+
labels such as `Works`, `HandlesErrors`, or `EdgeCases`.
44+
- Name subtests after the condition or outcome that distinguishes each case:
45+
`rejects missing port` or `preserves response body`, rather than `true`,
46+
`false`, or an index. Read the parent and subtest names together to check that
47+
a failure identifies what was expected.
48+
- For desktop tests, use natural-language names with the same subject and
49+
expected behavior, such as `parseAddress rejects an address without a port`.
50+
51+
## Make the test readable on its own
52+
53+
- Keep setup, the action under test, and expectations in a clear reading order.
54+
Separate them with blank lines when helpful; avoid comments that merely label
55+
obvious steps.
56+
- Use descriptive fixture and variable names. Keep the relevant inputs and
57+
expected values visible in the case so readers do not have to trace helpers
58+
to understand what it proves.
59+
- Avoid jargon in test comments where possible. Explain necessary domain terms
60+
or abbreviations on first use. For example, prefer `A stopped engine cannot
61+
accept new requests` over `Exercise the ineligible-node path`.
62+
- Use comments to explain why a condition matters, why an expectation follows,
63+
or why unusual setup is necessary. Do not narrate the code or describe an
64+
abandoned implementation.
65+
- Make failure messages identify the failed expectation and relevant case
66+
context. A reader should understand the failure without reconstructing an
67+
unrelated scenario.
3268

3369
## Share setup without hiding the cases
3470

71+
Prefer clarity over removing duplication. A little repeated setup is worthwhile
72+
when it makes each scenario understandable on its own. Extract helpers that
73+
remove distracting details while keeping the important inputs and expectations
74+
visible; avoid shared mutable fixtures that hide how those values are set.
75+
3576
For Go cases with the same setup and assertions, prefer a local helper closure
3677
that captures shared setup and calls `t.Run`. List explicit, named calls below
3778
it. A signature such as `test := func(name, input string)` often makes cases
3879
easier to read than a large table with mode-dependent branches.
3980

4081
Use ordinary tables when they are clearer. Separate acceptance and rejection
4182
helpers when that removes branching and makes expected outcomes explicit.
83+
For example, use `accept := func(name, input, expected string)` and
84+
`reject := func(name, input, wantMessage string)` for distinct outcomes.
4285
Define and list acceptance cases before rejection cases for consistency. Keep
4386
mutable state fresh per case so cases do not depend on execution order.
44-
Avoid adding a `wantError` parameter to a helper shared by acceptance and
45-
rejection cases; it forces every case through a conditional assertion path.
46-
Rejection helpers should check the relevant error field or returned error and
47-
its expected message or type. A parameter for the expected error message is
48-
appropriate in a rejection-only helper.
4987

5088
Mark Go assertion and setup helpers with `t.Helper()`. Pass the subtest's
5189
`*testing.T` to helpers that report failures so errors belong to the right case.
5290
Keep helpers local unless reuse across tests justifies a shared fixture.
5391

5492
## Check errors and structured results
5593

94+
- State expected results directly using concrete values where practical. Avoid
95+
computing expectations with the same logic or production helper being tested;
96+
that can reproduce the implementation's bug and let an incorrect result pass.
5697
- Check setup, file I/O, encoding, decoding, and operation errors. Do not discard
5798
an error just because the fixture is expected to be valid. Fail at the operation
5899
that failed, with enough context to diagnose it.
59-
- Prefer explicit `t.Fatalf` checks or a small test-only must-succeed helper that
60-
fails immediately. A `Must` helper is optional; avoid adding an abstraction
61-
when an explicit check is clearer.
100+
- Use `require.NoError` for setup or decoding that must succeed before the test
101+
can proceed. Use `assert.NoError` for other errors, including previously
102+
discarded JSON encoding/decoding errors, when continuation is safe. Keep added
103+
error checks separate from a pure assertion migration when requested.
62104
- Decode JSON and other structured output, then assert the relevant fields.
63105
Avoid `strings.Contains` as evidence that a serialized response or persisted
64106
configuration has the correct values. Check parsing errors before fields.
@@ -68,6 +110,53 @@ Keep helpers local unless reuse across tests justifies a shared fixture.
68110
of magic protocol values. Format multiline fixtures and header maps readably;
69111
run `gofmt` on changed Go tests.
70112

113+
## Use Testify concisely
114+
115+
- Prefer `assert` for independent expectations so a failure can report alongside
116+
later failures. Use `require` for prerequisites such as a non-nil pointer or
117+
sufficient slice length before dereferencing or indexing. Protocol states and
118+
cluster membership must also hold before continuing a handshake or making
119+
authenticated requests. When unsure whether continuation is safe, use
120+
`require`; preserving a fatal assertion is fine.
121+
- Prefer `assert.Equal` and `require.Equal` with expected values first. Avoid
122+
`EqualValues` when both arguments already have the same type. For numeric
123+
literals compared with typed values, use an explicitly typed expectation,
124+
for example `assert.Equal(t, int64(0), counter.Load())`. Reserve `EqualValues`
125+
for cases where comparison across types is part of the intended behavior.
126+
- Inline values used only once: `assert.Equal(t, "v8-version", versions["v8"])`
127+
and `require.NoError(t, json.Unmarshal(data, &result))`. Avoid a temporary
128+
`got` or `err` and a surrounding block just to make one assertion. Retain a
129+
variable when it is reused or makes a complicated operation easier to read.
130+
- Prefer direct comparisons of complete values over `assert.True` with
131+
`bytes.Equal` or `reflect.DeepEqual`. Compare slices directly with `Equal`
132+
where possible; avoid branching on an empty expected slice just to change
133+
assertion methods. Use `Empty` for an emptiness expectation instead of `Nil`
134+
unless the nil distinction is an explicit contract. Preserve both the non-nil
135+
and empty checks when a result must encode as `[]` rather than `null`, and use
136+
`Nil` when absence is part of the contract. If direct equality would
137+
distinguish nil from empty against the intended contract, normalize concisely
138+
or use `Empty` for that case.
139+
- Use `Same`/`NotSame` only when pointer identity is the behavior under test.
140+
Otherwise compare the contents or relevant fields; investigate an existing
141+
identity comparison before carrying it over mechanically.
142+
- Let Testify print the compared values. Add assertion messages only for useful
143+
context; avoid repeating actual/expected values in diagnostic arguments but you
144+
can explain the intended contract.
145+
Omit labels such as `"got"` when the comparison is self-explanatory.
146+
In loops without named subtests, include the case input when it helps locate
147+
the failure, for example `assert.True(t, isHostname(s), "hostname %q", s)`.
148+
Although such lack of named subtests should be considered for rewrite.
149+
150+
## Check effects before internal calls
151+
152+
Prefer checking returned values or resulting state over verifying that internal
153+
methods were called. When using mocks, avoid verifying every read or lookup;
154+
check what the code does with the returned information. Verify an interaction
155+
when it is itself part of the requirement, such as sending a notification,
156+
avoiding repeated requests when caching, or preserving a required ordering.
157+
Keep mock argument checks focused on values relevant to the behavior being
158+
proved, and verify other requirements in separate tests.
159+
71160
## Fit the existing test suite
72161

73162
For desktop tests, carry over the same focus, naming, error handling, and
@@ -78,5 +167,7 @@ Do not introduce a new framework or production-only test hooks for this style.
78167

79168
Run the relevant existing tests and required repository checks. In review,
80169
verify that each changed test has a clear failure condition, names the behavior
81-
it proves, checks its setup errors, and can be understood without tracing an
82-
unrelated scenario. State any verification that could not be run.
170+
it proves, checks its setup errors, and uses comments that explain intent in
171+
plain language. Confirm that its inputs and expectations can be understood
172+
without tracing an unrelated scenario. State any verification that could not
173+
be run.

‎AGENTS.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,15 @@ cross-process coverage. Some tests skip themselves rather than fail; refer to
9595
passing. Live engine and multi-node tests can modify real system state, so run
9696
them only when you intend to and only against systems and networks you control.
9797

98+
## Test readability
99+
100+
Follow [PAIR test style](.agents/skills/pair-test-style/SKILL.md) when writing,
101+
changing, or reviewing tests. Name Go tests `Test<Subject>_<ExpectedBehavior>`
102+
and consider three candidate names before selecting the clearest one. Use
103+
descriptive case names, keep inputs and expectations explicit, and avoid jargon
104+
in test comments where possible. Comments should explain intent or unusual setup.
105+
Apply this advice to tests in scope rather than starting unrelated rewrites.
106+
98107
## Checks
99108

100109
On Linux and macOS the repository-root `Makefile` is the shortest path to a clean

0 commit comments

Comments
 (0)