BloodHound icon indicating copy to clipboard operation
BloodHound copied to clipboard
verified publisher icon SpecterOps

BED-6873 Added Unit tests for AssetGroupTags

Open Useinovski opened this issue 4 weeks ago • 2 comments

Description

Some of the AGT tests were written using gomock.Any() , this does not properly test the functionality of the method as it’s forcing gomock to accept any argument and treat it as valid

We would like to remove instances of gomock.Any() where possible to further strengthen our unit tests

Describe your changes in detail

Motivation and Context

BED-6873

Resolves <TICKET_OR_ISSUE_NUMBER>

How Has This Been Tested?

Unit tests only, they all passed.

Please describe in detail how you tested your changes. Include details of your testing environment, and the tests you ran to see how your change affects other areas of the code, etc.

Screenshots (optional):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Database Migrations

Checklist:

  • [x] I have met the contributing prerequisites
    • Assigned myself to this PR
    • Added the appropriate labels
    • Associated an issue: https://github.com/SpecterOps/BloodHound/issues/672
    • Read the Contributing guide: https://github.com/SpecterOps/BloodHound/wiki/Contributing
  • [x] I have ensured that related documentation is up-to-date
    • Open API docs
    • Code comments (GoDocs / JSDocs)
  • [x] I have followed proper test practices
    • Added/updated tests to cover my changes
    • All new and existing tests passed

Summary by CodeRabbit

  • Tests
    • Improved determinism and reduced flakiness by replacing broad mock expectations with explicit values and structured query/filter parameters.
    • Strengthened validation of database and graph interactions in asset group tag tests, making behavior and results more predictable.
    • Aligned test setup to use shared constants for node and field identification.

✏️ Tip: You can customize this high-level summary in your review settings.

Useinovski avatar Dec 01 '25 20:12 Useinovski

Walkthrough

Replaces broad gomock.Any() matchers in asset group tags tests with concrete IDs, explicit SQLFilter/node-filter constructions using graphschema/common, deterministic cypher/query IDs, and specific mock return data; changes are confined to test wiring and mock expectations.

Changes

Cohort / File(s) Change Summary
Test Mock Expectations & Filters
cmd/api/src/api/v2/assetgrouptags_test.go		 Replaced generic gomock.Any() with explicit IDs (e.g., 1, 5, 1234, int64(-3)), concrete SQLFilter and pagination parameters, explicit nodeFilter keys using graphschema/common, and tightened PrepareCypherQuery, selector/tag/node, and configuration parameter mock expectations and return data for deterministic tests.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Areas to review:
    • Verify explicit ID and cypher ID values match intended test scenarios and fixtures
    • Confirm SQLFilter (filters, sorts, limits) and pagination semantics are correct
    • Check nodeFilter keys and graphschema/common constant usage for correctness
    • Validate updated mock return shapes (tags, selectors, nodes) against test assertions

Possibly related PRs

  • SpecterOps/BloodHound#2118 — Similar test updates replacing gomock.Any() with concrete IDs, filters, and graph-kind expectations in assetgrouptags tests.
  • SpecterOps/BloodHound#1798 — Introduces filtered-and-paginated selector DB calls and SQLFilter handling referenced by these tests.
  • SpecterOps/BloodHound#1972 — Related rewrites to assetgrouptags test wiring and selector/node mock patterns.

Suggested reviewers

  • irshadaj
  • urangel
  • wes-mil

Poem

🐰 I hopped through mocks with nimble feet,

Replacing Any() with IDs neat.
Filters set, selectors in line,
Tests now steady, passing fine.
A carrot cheer for deterministic treats!

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title is somewhat misleading—it says 'Added Unit tests' but the PR actually refactors existing tests by replacing gomock.Any() with concrete values rather than adding new tests. Revise the title to accurately reflect that this is a refactoring of existing tests, e.g., 'refactor: Replace gomock.Any() with concrete values in AssetGroupTags tests'.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description adequately explains the changes, motivation, testing approach, and includes the associated Jira ticket (BED-6873). The checklist is completed, though the 'Resolves' line uses a placeholder that wasn't filled in.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch BED-6873-2
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 98901f53377f43ab2f3f7c22000ec72c73f8f312 and 5fa8a871f5b3dd742718704e3e1ac41fcd809531.

📒 Files selected for processing (1)
  • cmd/api/src/api/v2/assetgrouptags_test.go (53 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings 
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.
📚 Learning: 2025-07-09T00:36:54.112Z 
Learnt from: mistahj67
Repo: SpecterOps/BloodHound PR: 1648
File: cmd/api/src/api/v2/assetgrouptags.go:763-766
Timestamp: 2025-07-09T00:36:54.112Z
Learning: In cmd/api/src/api/v2/assetgrouptags.go, the SearchAssetGroupTags method intentionally fetches all asset group tags and selectors without database-level filtering because it needs to build a complete `kinds` array from all relevant tags for the graph query filter. This allows members to be searched across all tags of the requested type while still filtering the returned tags/selectors by name match.

Applied to files:

  • cmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-07-22T20:30:34.839Z 
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.

Applied to files:

  • cmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-06-06T23:12:14.181Z 
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.

Applied to files:

  • cmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-11-05T21:13:08.542Z 
Learnt from: ktstrader
Repo: SpecterOps/BloodHound PR: 2020
File: cmd/api/src/queries/graph.go:198-203
Timestamp: 2025-11-05T21:13:08.542Z
Learning: In cmd/api/src/queries/graph.go, when ETAC filtering is enabled in GetAssetGroupComboNode, an empty etacAllowedList (when user.AllEnvironments is false and user.EnvironmentTargetedAccessControl is empty) is intentional and should not be guarded against. The empty list will cause query.In() to filter out all nodes, which is the correct security behavior—users with no environment assignments should see no results.

Applied to files:

  • cmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-09-08T19:22:49.284Z 
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx:34-35
Timestamp: 2025-09-08T19:22:49.284Z
Learning: In BloodHound's TagToZoneLabelDialog component (packages/javascript/bh-shared-ui/src/views/Explore/ExploreSearch/SavedQueries/TagToZoneLabelDialog.tsx), importing AssetGroupTag type from 'js-client-library' to type tag shapes is incorrect - this type should not be used for typing tags in this context.

Applied to files:

  • cmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-08-28T16:43:43.961Z 
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.

Applied to files:

  • cmd/api/src/api/v2/assetgrouptags_test.go
📚 Learning: 2025-06-17T22:37:36.389Z 
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.

Applied to files:

  • cmd/api/src/api/v2/assetgrouptags_test.go
🧬 Code graph analysis (1)
cmd/api/src/api/v2/assetgrouptags_test.go (6)
cmd/api/src/queries/graph.go (1)
  • PreparedQuery (413-418)
packages/go/graphschema/common/common.go (3)
  • Name (53-53)
  • Description (55-55)
  • ObjectID (52-52)
cmd/api/src/model/appcfg/parameter.go (1)
  • ScheduledAnalysis (47-47)
cmd/api/src/database/db.go (1)
  • ErrNotFound (41-41)
cmd/api/src/model/filter.go (1)
  • SQLFilter (98-101)
packages/go/schemagen/tsgen/tsgen.go (1)
  • ID (216-220)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build BloodHound Container Image / Build and Package Container
  • GitHub Check: build-ui
  • GitHub Check: run-analysis
  • GitHub Check: run-tests
🔇 Additional comments (5)
cmd/api/src/api/v2/assetgrouptags_test.go (5)

51-51: LGTM on import addition.

The graphschema/common import is correctly added to support explicit node property references (common.Name.String(), common.ObjectID.String()) in the refactored test filters.

435-444: Good replacement of gomock.Any() with concrete values.

The mock expectations now use explicit IDs and parameters, which strengthens the test by ensuring the correct arguments are passed to the mocked methods. The int64(-3) for PrepareCypherQuery appears to be a specific fitness bound constant.

1796-1804: Improved mock expectations for DeleteAssetGroupTagSelector.

The explicit model.AssetGroupTagSelector{AssetGroupTagId: 1} struct ensures the test validates that the correct selector is passed to the delete method, strengthening the test compared to using gomock.Any().

2948-2970: Well-structured test with explicit filter construction.

The test correctly builds the nodeFilter using explicit property references from graphschema/common and ensures consistency between myTags, the mock expectations, and the request body (tag_type: 1 for Tier).

3505-3514: Good explicit filter construction for history search tests.

The mock expectations now use explicit SQLFilter with the expected fuzzy search query and pq.StringArray parameters, ensuring the test validates proper filter construction in the search functionality.

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.

coderabbitai[bot] avatar Dec 01 '25 20:12 coderabbitai[bot]