redhat-developer / redhat-developer/rhdh-plugins

Add collector parameter contract assertion convention to scorecard AGENTS.md

Open Beginner friendly
#4,734 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

documentation ready-for-triage ready-to-code workspace/scorecard
Dominant language
TypeScript
Stars
48
Forks
120
Avg merge
2d 6h
Merged PRs (30d)
337

Description

What happened

In PR #4474, the review agent approved the test changes without flagging a test adequacy gap. Human reviewer gustavolira identified that the test mock for predecessor deployment collection only branched on input.from === DORA_PREDECESSOR_COLLECT_FROM, meaning that removing fetchItemsLimit: predecessorRange.fetchItemsLimit from the production code would leave the entire test suite green while production starts requesting 1000 deployments since 1970 on every DORA run.

The reviewer also noted that the catch/fallback path in collectSuccessfulProductionDeploymentsWithPreWindowBoundary had no test coverage — a transient rate limit during predecessor lookup would produce a misleading error message pointing at deployment data rather than the API.

The review agent's test assessment was limited to verifying that tests existed for the new functionality, not that they adequately covered the parameter contracts between callers and collectors.

What could go better

The scorecard workspace AGENTS.md has no guidance on what constitutes adequate test coverage for collector interactions. The existing mock pattern (mockResolvedValueOnce with conditional returns based on a single input field) makes it easy to add tests that pass without actually verifying the full contract between provider and collector.

The human reviewer caught this because they understood that fetchItemsLimit was the key behavioral difference between the normal collect and the predecessor collect — without it, the predecessor query degrades to an unbounded scan. This is a pattern-level issue: any time a provider passes structured input to a collector, tests should assert on the full input shape, not just the fields used for mock branching.

I am moderately confident this would help the review agent. The agent already reads and evaluates test files during review — giving it explicit criteria for what constitutes sufficient collector parameter testing would raise the bar from "tests exist" to "tests verify the contract." However, the agent's ability to reason about which parameters are behaviorally significant (vs. cosmetic) may still be limited without the pipeline context from proposal 1.

This proposal complements the pipeline guidance proposal — together, they give the agent both the domain context to understand why parameters matter and the testing convention to verify they're asserted.

Proposed change

Add a subsection to the proposed "DORA Collector Pipeline" section (or as a standalone section) in workspaces/scorecard/AGENTS.md titled "Testing conventions for collector interactions", covering:

  1. Assert full collector input contracts: When a provider calls collectorsService.collect(), tests must use toHaveBeenCalledWith (or equivalent) to assert on the full input object — including from, to, fetchItemsLimit, and any other parameters — not just the fields used for mock branching. Dropping a parameter from the production call must cause a test failure.

  2. Cover error/fallback paths: When collector calls are wrapped in try/catch with fallback behavior (e.g., falling back to fewer deployments on rate limit), add a test case using mockRejectedValueOnce to verify that (a) the fallback produces the correct output and (b) the warning message distinguishes the failure type.

  3. Review checklist item: When reviewing test changes for DORA providers, verify that mock assertions cover the distinguishing parameters of each collector call. Flag tests where the mock branches on one field but the production code sends additional fields that affect behavior.

Validation criteria

On the next 2 PRs that modify DORA provider tests in the scorecard workspace, the review agent should flag collector mock assertions that only branch on input.from without verifying other behavioral parameters like fetchItemsLimit or to. The review agent should also flag new try/catch blocks around collector calls that lack corresponding test coverage for the error path.


Generated by retro agent from https://github.com/redhat-developer/rhdh-plugins/pull/4474

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with workspaces/scorecard/AGENTS.md and review PR #4474, especially the collector mock and fallback discussion. Add the requested collector interaction testing subsection with full-input assertions, error-path coverage, and the review checklist item. Done means the guidance is present and matches the stated validation criteria for upcoming DORA provider test changes.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
documentation, testing-qa
Issue type
Documentation
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.