refactor(recipe): drop Cached*CountForTesting helpers (#1079) by mchmarny · Pull Request #1099 · NVIDIA/aicr

mchmarny · 2026-05-29T12:18:57Z

Summary

Removes the two _ForTesting count-based cache inspection helpers from pkg/recipe's exported surface and moves the only consumer into the internal aicr test package where it can use the per-DataProvider Contains helpers instead.

Motivation / Context

pkg/recipe is marked Public (evolving) in docs/integrator/public-api.md. The _ForTesting suffix on CachedStoreCountForTesting and CachedRegistryCountForTesting is convention-only; nothing prevents production callers from using them. The count helpers had a known limitation already documented in the source: their result is global across all DataProviders, so any parallel sibling test perturbs the baseline. Per-provider Contains*ForTesting helpers are scoped correctly and the existing test in aicr_internal_test.go already prefers them.

Fixes: #1079
Related: #1076 (epic), #1072 (PR that introduced the helpers)

Type of Change

Refactoring (no functional changes)

Component(s) Affected

Recipe engine / data (pkg/recipe)
Other: facade tests (aicr_test.go, aicr_internal_test.go)

Implementation Notes

Deleted CachedStoreCountForTesting (pkg/recipe/metadata_store.go) and CachedRegistryCountForTesting (pkg/recipe/components.go).
Moved TestClient_NoCacheGrowthAcrossManyCloseCycles from aicr_test.go (external aicr_test package) into aicr_internal_test.go (internal aicr package) so it can read the unexported Client.dp field directly.
Rewrote the assertion: instead of Count(before) → 50 cycles → Count(after) == before, the test now captures dp := c.dp before Close() (which zeros the field) and asserts recipe.CachedStoreContainsForTesting(dp) == false per iteration. Strictly stronger signal — fails on iteration 1 if eviction is broken, and per-provider scoping is robust to parallel sibling tests.
The new test also asserts the resolved *RecipeResult is non-nil per iteration (resolver contract).
The surviving Contains*ForTesting helpers remain exported but their docstrings were tightened to drop references to the deleted count helpers.

The remaining Contains*ForTesting exports are still convention-only test hooks; fully eliminating them is intentionally deferred (a follow-up could move them behind a build tag or relocate them to an internal subpackage). This PR halves the surface today with no production code changes.

Testing

make qualify  # passes

Affected tests pass under -race: TestClient_NoCacheGrowthAcrossManyCloseCycles, TestClient_CloseDrainsInflightResolve, TestClient_NoCachePopulationAfterClose.
Coverage delta:
- pkg/recipe: 90.8% → 91.3% (+0.5%, deleted uncovered exported helpers)
- github.com/NVIDIA/aicr: 63.3% → 63.3%

Risk Assessment

Low — Test-only refactor. No production code paths change. Public test helpers being removed had no callers outside this repo.

Rollout notes: N/A — external consumers cannot have been using these correctly (the suffix advertises test-only intent). If anyone was, they get a clear compile-time error and can switch to Contains*ForTesting.

Checklist

Tests pass locally (make test with -race)
Linter passes (make lint)
I did not skip/disable tests to make CI green
I added/updated tests for new functionality
I updated docs if user-facing behavior changed (N/A — internal test surface)
Changes follow existing patterns in the codebase
Commits are cryptographically signed (git commit -S)

Deletes the two count-based test inspection helpers from pkg/recipe's exported surface. They were convention-only test hooks (the _ForTesting suffix is not enforced) and the only call site (TestClient_NoCacheGrowthAcrossManyCloseCycles) was a baseline-diff assertion that conflated this test's cache growth with sibling tests'. The moved test now lives in the internal aicr test package where it can read Client.dp directly. It captures each iteration's DataProvider before Close and asserts Contains(dp) == false per iteration — a strictly stronger, sibling-test-independent signal than the previous cumulative count delta. The surviving Contains*ForTesting helpers remain exported (per-provider scope, no global cache visibility) and are documented for the same narrow purpose. Refs #1079

coderabbitai · 2026-05-29T12:21:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 97d10281-21eb-490a-b5d4-95a23bae4b6c

📥 Commits

Reviewing files that changed from the base of the PR and between 9a31cc0 and 6ce9da2.

📒 Files selected for processing (4)

aicr_internal_test.go
aicr_test.go
pkg/recipe/components.go
pkg/recipe/metadata_store.go

💤 Files with no reviewable changes (1)

aicr_test.go

📝 Walkthrough

Walkthrough

This PR removes count-based test helpers (CachedStoreCountForTesting and CachedRegistryCountForTesting) from the public pkg/recipe API surface in metadata_store.go and components.go. It updates documentation for the remaining per-provider helper functions to clarify their purpose. The regression test TestClient_NoCacheGrowthAcrossManyCloseCycles is relocated from aicr_test.go to aicr_internal_test.go to verify cache eviction using only the per-provider helpers, and unused imports are cleaned up accordingly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly summarizes the main change: removing Cached*CountForTesting helpers from the pkg/recipe public API.
Description check	✅ Passed	Description is detailed and directly related to the changeset, explaining the motivation, implementation, and testing of the refactoring.
Linked Issues check	✅ Passed	The PR fully addresses issue `#1079` by removing the count-based helpers and moving the test to use per-DataProvider Contains helpers instead, meeting the third acceptance criteria.
Out of Scope Changes check	✅ Passed	All changes are directly in scope: deletion of the two count helpers, relocation and rewrite of one test, and documentation updates to remaining helpers.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch refactor/1079-cachetest-helpers

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-05-29T12:29:45Z

Coverage Report ✅

Metric	Value
Coverage	77.1%
Threshold	75%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-77.1%25-green)

Merging this branch will increase overall coverage

Impacted Packages	Coverage Δ	🤖
github.com/NVIDIA/aicr/pkg/recipe	91.35% (+0.50%)	👍

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/NVIDIA/aicr/pkg/recipe/components.go	89.62% (+4.04%)	106 (-5)	95	11 (-5)	👍
github.com/NVIDIA/aicr/pkg/recipe/metadata_store.go	87.89% (+1.22%)	355 (-5)	312	43 (-5)	👍

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

mchmarny requested a review from a team as a code owner May 29, 2026 12:18

mchmarny added the enhancement label May 29, 2026

github-actions Bot added the size/M label May 29, 2026

mchmarny self-assigned this May 29, 2026

mchmarny enabled auto-merge (squash) May 29, 2026 12:24

lalitadithya approved these changes May 29, 2026

View reviewed changes

mchmarny merged commit 8d4de99 into main May 29, 2026
33 of 34 checks passed

mchmarny deleted the refactor/1079-cachetest-helpers branch May 29, 2026 12:29

coderabbitai Bot mentioned this pull request May 29, 2026

refactor(recipe,validator): per-DataProvider sources + criteria-registry de-globalization #1107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(recipe): drop Cached*CountForTesting helpers (#1079)#1099

refactor(recipe): drop Cached*CountForTesting helpers (#1079)#1099
mchmarny merged 1 commit into
mainfrom
refactor/1079-cachetest-helpers

mchmarny commented May 29, 2026

Uh oh!

coderabbitai Bot commented May 29, 2026

Walkthrough

Estimated code review effort

Uh oh!

Uh oh!

github-actions Bot commented May 29, 2026

Changed files (no unit tests)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mchmarny commented May 29, 2026

Summary

Motivation / Context

Type of Change

Component(s) Affected

Implementation Notes

Testing

Risk Assessment

Checklist

Uh oh!

coderabbitai Bot commented May 29, 2026

Walkthrough

Estimated code review effort

Uh oh!

Uh oh!

github-actions Bot commented May 29, 2026

Coverage Report ✅

Merging this branch will increase overall coverage

Changed files (no unit tests)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants