Skip to content

chore: Lazy mock initialization in artifact controller tests#101

Merged
bupd merged 3 commits intomainfrom
test/lazy-mock-init-artifact-controller
Apr 10, 2026
Merged

chore: Lazy mock initialization in artifact controller tests#101
bupd merged 3 commits intomainfrom
test/lazy-mock-init-artifact-controller

Conversation

@Vad1mo
Copy link
Copy Markdown
Contributor

@Vad1mo Vad1mo commented Apr 4, 2026

Summary

  • Replace eager initialization of all 11 mocks in SetupTest() with per-mock setupXxx() helpers
  • Each test now initializes only the mocks it actually uses, reducing total mock.Mock instances from ~352 to ~89 (75% reduction)
  • Proportionally lowers sync.Mutex instrumentation overhead under the race detector

Related Issues

Closes #100 (Option 1: lazy mock initialization)

Type of Change

  • Tests (test:)

Design

SetupTest() previously created all 11 mock objects and wired them into the controller for every test method (19 tests + 13 mid-test resets = 32 calls × 11 mocks = 352 instances). Many tests only use 1–3 mocks.

The new approach:

  1. SetupTest() nils all mock fields and creates a bare controller{}
  2. 11 setupXxx() helpers each create a fresh mock and wire it into the controller
  3. Each test calls only the helpers it needs
  4. Mid-test resets (c.SetupTest() + specific setup calls) replace the previous blanket reinitialization

Mock creation count per test (before → after):

Test Before After
TestPopulateIcon 11 0
TestCount 11 1
TestGetAddition 11 1
TestAddTo 11 1
TestRemoveFrom 11 1
TestIsInto 11 1
TestGet 11 2
TestWalk 11 2
TestDeleteDeeply 77 (7×11) 29
TestCopy 11 7
Total 352 ~89

Testing

  • All 19 tests pass (go test -count=1)
  • All 19 tests pass with race detector (go test -race)
  • No new warnings introduced

Checklist

  • PR title follows Conventional Commits format
  • Commits are signed off (git commit -s)
  • No new warnings introduced

Summary by CodeRabbit

  • Tests
    • Refactored test setup to use a minimal test controller and modular per-dependency setup helpers, making tests leaner and enabling each test to initialise only the mocks it needs.

Note: This release contains no user-facing changes. Updates are internal testing improvements.

Replace eager initialization of all 11 mocks in SetupTest() with
per-mock setup helpers. Each test now initializes only the mocks it
actually uses, reducing total mock.Mock instances from ~352 to ~89
(75% reduction) and proportionally lowering race-detector overhead.

Closes #100

Signed-off-by: Vadim Bauer <vb@container-registry.com>
Copilot AI review requested due to automatic review settings April 4, 2026 19:34
@github-actions github-actions Bot added the tests label Apr 4, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 565f2a8f-bdcb-4089-8711-174088bca91b

📥 Commits

Reviewing files that changed from the base of the PR and between 893a983 and 1d34f42.

📒 Files selected for processing (1)
  • src/controller/artifact/controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/controller/artifact/controller_test.go

📝 Walkthrough

Walkthrough

Reworked the controller/artifact test suite setup: SetupTest() now resets all controller dependency fields and creates a bare controller. Introduced per-dependency setup helpers that allocate mocks/fakes and assign them to c.ctl. Tests now call only the helpers they need; setupProCtl() was changed to assign only c.ctl.proCtl.

Changes

Cohort / File(s) Summary
Test setup refactoring
src/controller/artifact/controller_test.go
Replaced eager instantiation of all mocks in SetupTest() with a bare c.ctl = &controller{} and nilified dependency fields. Added per-dependency helpers (setupRepoMgr, setupArtMgr, setupArtrashMgr, setupBlobMgr, setupTagCtl, setupLabelMgr, setupAbstractor, setupImmutableMtr, setupRegCli, setupAccMgr, setupProCtl) that allocate and assign mocks/fakes to c.ctl. Updated tests to invoke only required setup helpers and adjusted setupProCtl() to set only c.ctl.proCtl rather than rebuilding the controller.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'test: Lazy mock initialization in artifact controller tests' clearly and specifically summarizes the main change: refactoring tests to lazily initialize mocks instead of eagerly creating all of them.
Description check ✅ Passed The description follows the template well with all key sections: Summary, Related Issues, Type of Change, Design explanation with detailed table, Testing checklist, and PR Checklist. Information is complete and comprehensive.
Linked Issues check ✅ Passed The PR successfully implements Option 1 from issue #100: lazy mock initialization. The code changes reduce mock instances from 352 to ~89 (75% reduction) and address the race-detector overhead issue with minimal code changes.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to the artifact controller test file, implementing only the lazy mock initialization pattern outlined in issue #100. No unrelated refactoring, dependency updates, or side changes are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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 test/lazy-mock-init-artifact-controller

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 4, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA f0fb2d8.
Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the controller/artifact test suite to reduce testify/mock + race detector overhead by switching from eager initialization of all mocks in SetupTest() to per-test lazy initialization via dedicated setupXxx() helpers.

Changes:

  • Refactored SetupTest() to create only a bare controller{} and reset mock fields to nil.
  • Added setupXxx() helper methods to initialize and wire individual mocks into the controller on demand.
  • Updated each test (and mid-test resets) to initialize only the mocks it actually uses.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

@bupd bupd self-assigned this Apr 7, 2026
@github-actions
Copy link
Copy Markdown

Preview images for this PR are available in registry.goharbor.io/harbor-next with tag pr-101.

  • registry.goharbor.io/harbor-next/harbor-core:pr-101
  • registry.goharbor.io/harbor-next/harbor-jobservice:pr-101
  • registry.goharbor.io/harbor-next/harbor-registryctl:pr-101
  • registry.goharbor.io/harbor-next/harbor-exporter:pr-101
  • registry.goharbor.io/harbor-next/harbor-portal:pr-101
  • registry.goharbor.io/harbor-next/harbor-registry:pr-101
  • registry.goharbor.io/harbor-next/harbor-trivy-adapter:pr-101

Verify a preview image:

cosign verify \
  --certificate-identity-regexp="https://github.com/container-registry/harbor-next/.github/workflows/pr-ci.yml@.*" \
  --certificate-oidc-issuer="https://token.actions.githubusercontent.com" \
  registry.goharbor.io/harbor-next/harbor-core:pr-101

Verify SBOM attestation:

cosign verify-attestation \
  --certificate-identity-regexp="https://github.com/container-registry/harbor-next/.github/workflows/pr-ci.yml@.*" \
  --certificate-oidc-issuer="https://token.actions.githubusercontent.com" \
  --type spdxjson \
  registry.goharbor.io/harbor-next/harbor-core:pr-101

@bupd bupd changed the title test: Lazy mock initialization in artifact controller tests chore: Lazy mock initialization in artifact controller tests Apr 10, 2026
@bupd bupd merged commit c1a4bdc into main Apr 10, 2026
46 of 47 checks passed
@bupd bupd deleted the test/lazy-mock-init-artifact-controller branch April 10, 2026 11:49
Vad1mo added a commit that referenced this pull request Apr 12, 2026
The lazy mock initialization PR (#101) made SetupTest() reset all
mocks to nil; tests must call setupXxx() helpers afterward. The
fourth subtest in TestEnsureArtifact called SetupTest but never
re-initialised c.repoMgr/c.artMgr/c.accMgr/c.abstractor, causing
a nil pointer panic on the first .On() call.

The panic was previously masked by the TestCopy infinite-recursion
OOM (fixed by setting UTTEST=true in test-unit env).

Signed-off-by: Vadim Bauer <vb@container-registry.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: testify/mock race-detector overhead causes ~13min CPU time in controller/artifact

3 participants