Add "testing" review stage - #485
Closed
kees wants to merge 2 commits into
Closed
Conversation
Nothing in the prompt set asks whether a change is testable, or how to review a test that a patch brings with it. selftests.md covered the selftest build system; KUnit was absent entirely. Add testing.md for the general case: when a missing test is worth raising and when it is routine, and how to judge a test that is present. Add kunit.md for the framework semantics that make a test silently wrong rather than merely unidiomatic. Extend selftests.md with the result protocol and the harness APIs. Neither framework guide restates naming, Kconfig entries or file placement. Documentation/dev-tools/kunit/style.rst and Documentation/dev-tools/ kselftest.rst carry those in the tree under review, where they cannot drift, so the guides instead enumerate the questions those documents answer and require each to be answered against them. Both phrase some rules as recommendations, which the guides call out, since a review honouring only the requirements skips file placement. These belong upstream in the review-prompts project. Until they land there this vendored snapshot has diverged, so REVISION carries a local suffix. That suffix is load-bearing: the bundle installs to a directory named for REVISION and short-circuits on a completion marker, so an existing install would otherwise never see the new files. Signed-off-by: Kees Cook <kees@kernel.org>
The analysis stages cover intent, implementation, control flow, resources, locking, security and hardware. None asks whether a change is testable, or reviews the tests a patch brings with it. Add a test engineer persona to the parallel analysis fan-out. It runs for every patch rather than being chosen by the planning stage, since a change that should have had a test and does not is the case with nothing to match on. It goes in at 8, ahead of the consolidation stages, which renumber to 9 through 12. Nothing persists a stage number: the "stage" field attached to a concern is read only by the deduplication prompt, to say which analyst raised it. Inserting it there rather than appending keeps the numbering in execution order. The general testing guide loads for every patch; the framework guides load when the patch text names that framework. All three are kept out of the pre-screen's broadcast set, so they reach this stage alone. The same triggers name the framework's in-tree style document, which is read from the worktree and rendered into the prompt rather than left for the stage to fetch: asked to fetch it, the stage answered without calling a tool at all. A tree without the document contributes nothing. The stage also receives the follow-up series context that verification already gets, since where a test belongs in a series is only answerable from the patches that come after it. Signed-off-by: Kees Cook <kees@kernel.org>
Contributor
Author
|
I've got a much better series coming. Closing this in favor of that one. |
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
1 participant
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.
This adds a missing review stage persona: "testing". There are a few interesting pieces here, and I'm not sure the right way to approach them:
The style guides need to be included in the prompt verbatim, and asking the LLMs nicely to read the files didn't seem to work, so I explicitly inject their contents. This creates a dependency on the harness for the prompts if they get moved to the upstream prompts collection.
I had to renumber everything. I think this is okay, but wow was it a lot of churn. Maybe the stages need to be refactored in some way to avoid the explicit numbering?
What do folks think about this approach? It worked well for a few series I tested it on.