Repository navigation
Agents are bad at writing tests #4144
Description
Description
I was looking at the tests in #4143 and I realised once again how bad agents are at testing what they implement.
We need tests that actually test that we fix things, not that we have implemented what we have implemented.
For example:
There are at least two tests currently merged in main that provide little or no coverage of the behavior they claim to test.
- test/test_rlhf.py::test_get_dataloader
The test parametrizes infinite=[True, False], but both branches make the exact same assertion:
if infinite:
assert not is_tensor_collection(dl)
else:
assert not is_tensor_collection(dl)
As a result, the infinite parametrization provides no coverage of the actual distinction between the two modes.
The documented behavior is that infinite=True wraps the replay buffer in create_infinite_iterator, whereas infinite=False returns the finite replay buffer directly.
The test should check that behavior instead:
- with infinite=False, exhaust the iterator and verify it raises StopIteration;
- with infinite=True, consume more than one dataset epoch and verify iteration continues.
Ideally the test should compute the number of batches in one epoch and request n_batches + 1 batches from the infinite iterator, making the check deterministic and cheap.
- test/objectives/test_controllers.py::test_deterministic_with_zero_variance
This test calls RBFController twice with the same inputs and zero covariance and verifies that the resulting action means are equal.
However, RBFController.forward() contains no sampling. It is deterministic regardless of the input covariance. Calling any deterministic implementation twice with identical inputs will therefore satisfy this test.
Consequently, the test does not actually verify the zero-variance behavior its name suggests.
A meaningful behavioral test would verify that zero input uncertainty produces essentially zero output uncertainty, e.g. by checking the returned action covariance against zero within the tolerance required by the controller’s intentional numerical jitter.
Something along these lines:
def test_zero_input_covariance_collapses_action_covariance(self):
controller = RBFController(
input_dim=4, output_dim=1, max_action=1.0, n_basis=5
).double()
mean = torch.randn(2, 4, dtype=torch.float64)
zero_cov = torch.zeros(2, 4, 4, dtype=torch.float64)
action_mean, action_cov, _ = controller(mean, zero_cov)
assert torch.isfinite(action_mean).all()
torch.testing.assert_close(
action_cov,
torch.zeros_like(action_cov),
atol=2e-6,
rtol=0,
)
The exact tolerance should account for the intentional 1e-6 diagonal jitter in the implementation.
General principle
Regression tests should preferably assert the externally observable behavior that was broken, rather than an implementation detail or a property that would hold independently of the code path being tested.
This makes the test answer the important question: if the underlying bug is reintroduced, will this test actually fail?
We need to audit the repo for this kind of low quality test coverage, come back to the PR that added them, and replace them with more sensible ones.
Activity
quinnarnold commentedon Aug 24, 2026
/assign
vmoens commentedon Aug 24, 2026
Reopening as we still need an audit of the repo for such tests
quinnarnold commentedon Aug 24, 2026
After doing an audit review per your suggestion, these are the findings so far. I have not made any additional changes yet. I also checked adjacent coverage and open PRs before including these.
Findings
prefill_replay_buffer: the non-stringchunk_sizetest checks only final buffer length. Ignoring chunking and copying everything at once would still pass. A stronger test could record copy sizes and verify that multiple calls occur, each is at mostchunk_size, and their total equalsn_samples.Reward2GoTransform: the underlyingreward2go()formula has exact coverage elsewhere, but the transform and replay-buffer integration paths only check shapes or nonzero values. Returning any same-shaped nonzero tensor would satisfy them. A small deterministic sequence could verify that these paths produce the same values asreward2go().UCB1TunedScore: the tests check shape and finiteness but not the documented variance formula ormin(0.25, V_i)clamp. Hand-computed low- and high-variance cases would distinguish both paths.NoisyLinearfactorized noise: the current test checks marginal mean and standard deviation, which independent elementwise noise could also satisfy. A deterministic structural assertion could verify the intended outer-product factorization and replace the current 1,000-sample loop.reset_noise: the helper is intended to be used as aModule.applycallback, but the test calls it directly on annn.Sequential, where it does nothing. The assertion only confirms that the layers already contain initialized noise. The test could usenetwork.apply(reset_noise)and compare every noisy layer’s epsilon buffers before and after.RNDLoss.update_fraction: both configurations are only checked for scalar output, so internally treating every fraction as1.0would still pass. Deterministic per-sample losses or a controlled mask could verify the selected samples.Prioritized replay reduction:
test_rb_trajectoriesparameterizesreductionbut never passes it to the buffer.test_replay_buffer_trajectoriespasses it, but all modes make the same shape and positive-weight assertions.Fixed priorities whose min, max, mean, and median differ would verify the public reduction wiring.
BCLoss: generic reduction behavior is covered centrally, but these tests do not numerically distinguish L1, L2, and smooth-L1. Mapping all three to MSE would still pass. A fixed actor and target could provide exact expected values.DiffusionBCLoss: thenonetest only checks key presence, while mean and sum only check finiteness. Hardcoding mean reduction would still pass. Controlled noise and score outputs could verify the elementwise, mean, and sum relationship.ACTLoss: the decomposition test validly checkskl_weightwiring, but its expected value is constructed from components returned by the same implementation. A fixed-output actor could independently verify the documented L1 reconstruction and Gaussian KL formulas while retaining the weighting check.SFTLoss: the current matrix provides useful execution and shape coverage, but it does not numerically distinguish sequence normalization, mean versus sum, or MinorSFT from ordinary SFT. A smaller deterministic oracle could cover those options while retaining one integration smoke test.GRPOLossasymmetric clipping: the DAPO and symmetric configurations are only checked for output structure, finiteness, and valid ranges. Treating(0.20, 0.28)as symmetric(0.20, 0.20)would still pass. A deterministic ratio above the symmetric upper bound would distinguish them.kl_mask_threshold: the test deliberately chooses a threshold high enough that no token is masked. Removing the masking branch would therefore still pass. A deterministic case with one token above and one below the threshold could verify aggregation.CISPOLoss: the test checks output structure and finiteness but does not distinguish CISPO’s clipped-importance objective from ordinary GRPO. A fixed ratio and advantage where the objectives differ would provide that coverage.Secondary items
ClipPPOLossasymmetric clipping: the existing test meaningfully covers buffer registration, bounds, scheduling, and equal-bound equivalence. The remaining gap is only that the asymmetric public objective is called without asserting its result.The TD-lambda and GAE tensor-lambda matrices appear to contain the same typo: the code checks
gamma_tensorwhen deciding whether to constructlmbda_vec, so thelmbda_tensorcases do not select what their test IDs claim. Tensor lambda is already supported by the implementation, so this looks like a small dead-parameterization correction rather than a contract question.Suggested grouping
I don't think all of these should go into one large PR. A few natural groups seem to be:
network.apply(reset_noise).I'd be inclined to start with the two NoisyLinear tests. They're in one file, cover the same component, don't need optional dependencies, and replacing the 1,000-sample statistical test should also make that part of the suite faster.
One question
I'm also unsure about the intended behavior of
MLP.single_bias_last_layer. PassingTruehas raisedNotImplementedErrorsince the initial implementation, but the argument is still public and the documentation describes it as supported. The test is parameterized overTrueandFalse, but always passesFalse.Should this option actually be implemented, or should the API, documentation, and tests make it explicit that it is unsupported?
Does this breakdown make sense? If so, I can start with the NoisyLinear pair and keep the other groups in separate PRs. If you'd rather prioritize a different group, let me know.
vmoens commentedon Aug 26, 2026
Please go ahead with the refactoring if that is something you want to do!
Ping me or @theap06 for reviews.
RE
MLP.single_bias_last_layerit looks like a bug that should be fixed. I would try to implement the feature to follow what the doc says and, if you hit a wall, find a suitable solution like deprecation etc.