docs: add test discipline rules to agent guidelines - #8372
Conversation
AI coding agents increasingly contribute tests alongside features. A common failure mode is tests coupled to the implementation under test (assertions on private internals, mirror-image references, mocks of internals) that pass trivially and give false coverage confidence. Codify contract-testing rules so agent-authored tests verify behavior through stable interfaces instead. CLAUDE.md kept in sync with AGENTS.md as required by the file header. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b2d1163c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - Comments should explain **why**, not **what**. Describe the purpose and reasoning, not the mechanics that the code already shows. | ||
| - New features must include corresponding tests and documentation updates. | ||
|
|
||
| ### Test discipline |
There was a problem hiding this comment.
This is a non-merge commit, but its commit message contains no Signed-off-by trailer. Add the author sign-off so the commit satisfies the repository's mandatory contribution requirement.
AGENTS.md reference: AGENTS.md:L8-L8
Useful? React with 👍 / 👎.
|
Thank you for adding this! Do we want to add something like: if the change is substantially large, add integration test? i.e. if we make changes on optimizer, instead of just unit test the optimizer, we should add a minimal training loop with a SimpleModel like training-framework-benchmarking-v5/third_party/DeepSpeed/tests/unit/checkpoint/test_universal_checkpoint.py |
Hi @pengdurice , add integration test is a good idea. Let me add it. Thanks! |
… changes Address review feedback: a contract change observable only at the training-loop or inference level (e.g. optimizer behavior) needs an integration test, not just unit tests. CLAUDE.md kept in sync with AGENTS.md as required by the file header. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
|
Hi @pengdurice , I added a line for integration test and tried to clearly define substaintial change. Can you take a look? Thanks! |
Make the trigger an affirmative, external check (does the change affect the external contract at the training-loop or inference level?) instead of a negative one (observable only at that level), with no size or complexity vocabulary. CLAUDE.md kept in sync with AGENTS.md. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
pengdurice
left a comment
pengdurice
left a comment
There was a problem hiding this comment.
one last question, does the agent read the PR description? I think so? do we need to add requests of adding verification on at least some hardware when the change is substantial?
Address review feedback: executing the integration test and reporting the hardware spec and results in the PR is what makes it evidence, so require both. CLAUDE.md kept in sync with AGENTS.md. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
| - Anchor to an external oracle or an independently derived reference instead of re-implementing the logic under test. | ||
| - Mocks must stand in for a collaborator's documented contract (schema, protocol), never for internals of the module under test. | ||
| - Changes that affect the external contract at the training-loop or inference level require integration tests, not just unit tests (e.g. a minimal training loop with `SimpleModel`). | ||
| - Integration tests must be executed on an actual device, not merely written: report the execution hardware spec and results in the PR. |
There was a problem hiding this comment.
nit: an actual device --> actual devices. Thank you!
I believe most morden agents would be able to get PR description with tool call. I add a line to require agents to report the hardware spec and result used for local validation when submit integration test. |
Review nit: an actual device -> actual devices. Signed-off-by: Ma, Guokai <guokai.ma@intel.com>
pengdurice
left a comment
pengdurice
left a comment
There was a problem hiding this comment.
Thank you for adding this test rules!
2 participants
Motivation
AI coding agents increasingly submit tests alongside features, and a recurring failure mode of such contributions is tests coupled to the implementation rather than the contract:
Such tests pass trivially, break on harmless refactors (or worse, survive bugs they should catch), and give false coverage confidence.
What this PR adds
A
### Test disciplinesection to the agent guidelines (AGENTS.md+CLAUDE.md, kept in sync as the file header requires) with operational rules:The rules are deliberately example-free to keep the guide lean.