added tests for helpers and init common lib

[emmanuelrobles]

Description

Added more UT

Motivation and Context

Increase test coverage

How Has This Been Tested?

Running locally

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)

Checklist:

• Documentation updates are needed, and have been made accordingly.
• I have added and/or updated tests to cover my changes.
• All new and existing tests passed.
• My changes include a database migration.

Summary by CodeRabbit

• Tests
  • Added comprehensive unit tests for domain-name, SID, and LDAP-time helpers covering conversions, parsing, filtering, and error cases.
  • Added initialization tests for the library cache lifecycle: initial creation, using a provided instance, and handling reinitialization with warnings.
  • Introduced a non-parallel test collection and ensured cache isolation/wiring for reliable, isolated execution of cache-dependent tests.

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

[coderabbitai]

Walkthrough

Adds multiple unit tests for helper functions (domain/SID/LDAP-time), new tests for CommonLib initialization and cache behavior, a test collection definition to disable parallelization, and cache-isolation wiring in an existing test class.

Changes

Cohort / File(s)	Change Summary
Helper Method Tests test/unit/CommonLibHelperTests.cs	Added using System.Security.Principal and multiple unit tests covering DomainNameToDistinguishedName, ConvertSidToHexSid (valid/invalid), IsSidFiltered (case-insensitive well-known SIDs, prefix checks, negatives), and ConvertLdapTimeToLong (null -> -1, invalid -> FormatException, valid parse).
CommonLib Initialization Tests test/unit/CommonLibTests.cs	New test class CommonLibTests with tests that (1) first initialization without a cache creates and populates the Cache singleton, (2) using a provided Cache instance is honored, and (3) a second initialization logs a warning and does not replace the existing cache. Includes private reflection helper ResetCommonLibState.
Test Collection Definition test/unit/CollectionDefinitions/CacheTestsCollectionDefinition.cs	New public collection definition CacheTestCollectionDefinition annotated with [CollectionDefinition(nameof(CacheTestCollectionDefinition), DisableParallelization = true)] to disable parallelization for cache-dependent tests.
Cache Isolation in Existing Tests test/unit/UserRightsAssignmentProcessorTest.cs	Added [Collection(nameof(CacheTestCollectionDefinition))] to the test class, imported the collection namespace, and reset cache in the test constructor via Cache.SetCacheInstance(null) for isolation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

• Inspect reflection-based ResetCommonLibState for correct access/cleanup and potential side effects.
• Verify the warning log expectation and that reinitialization does not replace the cache.
• Validate test vectors for SID-to-hex conversion and LDAP-time parsing edge cases.
• Confirm collection definition and [Collection] usage correctly serialize tests that share cache state.

Suggested reviewers

• definitelynotagoblin
• MikeX777

Poem

🐇 I hop through tests with tiny paws,
Dots become DCs without a pause,
SIDs turn to hex, times parsed just right,
Caches reset snugly through the night,
CI hums softly — carrot-bright.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding unit tests for helper functions and CommonLib initialization.
Description check	✅ Passed	The description covers most required sections including motivation, testing approach, and type of change with checklist items, though testing details are minimal.

✨ 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 feature/BED-6972

---

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0609651 and ca20c6c.

📒 Files selected for processing (3)

• test/unit/CollectionDefinitions/CacheTestsCollectionDefinition.cs (1 hunks)
• test/unit/CommonLibTests.cs (1 hunks)
• test/unit/UserRightsAssignmentProcessorTest.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• test/unit/CollectionDefinitions/CacheTestsCollectionDefinition.cs

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-07-24T21:38:13.462Z

Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 229
File: .github/workflows/git-dev.yml:35-39
Timestamp: 2025-07-24T21:38:13.462Z
Learning: In the SpecterOps/SharpHoundCommon repository, the team prefers using `sleet push --force` to overwrite git-dev packages rather than creating unique versions with commit SHAs, as they want a single "current state" package for downstream consumers.

Applied to files:

• test/unit/UserRightsAssignmentProcessorTest.cs

📚 Learning: 2025-10-17T13:43:46.833Z

Learnt from: MikeX777
Repo: SpecterOps/SharpHoundCommon PR: 241
File: src/CommonLib/Processors/LdapPropertyProcessor.cs:168-169
Timestamp: 2025-10-17T13:43:46.833Z
Learning: Properties added to dictionaries returned by methods in SharpHoundCommon (such as those in LdapPropertyProcessor) may be consumed by dependent projects like SharpHound (SH) and SharpHoundEnterprise (SHE), even if they are not used within the SharpHoundCommon repository itself.

Applied to files:

• test/unit/UserRightsAssignmentProcessorTest.cs

🧬 Code graph analysis (2)

test/unit/UserRightsAssignmentProcessorTest.cs (2)

test/unit/CommonLibTests.cs (1)

• Collection (11-91)

src/CommonLib/Cache.cs (1)

• SetCacheInstance (151-155)

test/unit/CommonLibTests.cs (3)

src/CommonLib/Logging/Logging.cs (2)

• Logging (7-20)
• ILogger (31-34)

src/CommonLib/CommonLib.cs (2)

• CommonLib (5-46)
• InitializeCommonLib (15-36)

src/CommonLib/Cache.cs (1)

• SetCacheInstance (151-155)

⏰ 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). (1)

• GitHub Check: build

🔇 Additional comments (7)

test/unit/UserRightsAssignmentProcessorTest.cs (2)

21-21: Good practice: collection-based test isolation.

Applying the collection attribute ensures these cache-sensitive tests run serially, preventing race conditions on the Cache singleton.

---

26-32: Cache reset pattern ensures test isolation.

Resetting the cache in the constructor prevents state leakage between tests. This aligns with the isolation strategy used in the test collection.

test/unit/CommonLibTests.cs (5)

11-18: Well-structured test isolation setup.

The collection attribute and state reset in the constructor ensure each test starts with a clean CommonLib and Cache state.

---

20-34: Test correctly verifies default cache initialization.

The test confirms that calling InitializeCommonLib() without arguments creates a Cache instance with all internal dictionaries properly initialized.

---

36-47: Test correctly verifies custom cache instance usage.

Using Assert.Same properly validates that the provided cache instance becomes the active singleton.

---

49-74: Test correctly validates idempotent initialization behavior.

The test properly verifies that subsequent initialization calls preserve the first cache instance and log a warning. The mock verification correctly checks for the warning message.

---

76-90: Reflection-based reset provides necessary test isolation.

The helper appropriately resets both the private _initialized flag and the cache singleton. While reflection is brittle, it's an acceptable tradeoff for test isolation when no public reset API exists.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

test/unit/CommonLibTests.cs (1)

74-88: Consider adding a test-only reset method to CommonLib.

The reflection-based reset is necessary for test isolation but fragile—it depends on private implementation details and will break if the field name or visibility changes. Additionally, setting the cache to null could cause issues in concurrent test scenarios.

Consider adding a conditional compilation directive in the production code:

#if DEBUG || TESTING
public static void ResetForTesting()
{
    _initialized = false;
    Cache.SetCacheInstance(null);
}
#endif

This would make tests more maintainable while keeping the reset logic out of production builds.

test/unit/CommonLibHelperTests.cs (1)

299-304: Consider adding edge case coverage.

The test correctly verifies the basic conversion, but consider adding test cases for edge cases:

• Single component domain (e.g., "local" → "DC=local")
• Multi-level domains (e.g., "sub.test.local" → "DC=sub,DC=test,DC=local")
• Empty or null input (if applicable)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 25ca222 and 5b982f3.

📒 Files selected for processing (2)

• test/unit/CommonLibHelperTests.cs (2 hunks)
• test/unit/CommonLibTests.cs (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-06-26T16:37:09.005Z

Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 217
File: src/CommonLib/Timeout.cs:21-21
Timestamp: 2025-06-26T16:37:09.005Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team prefers to use TaskScheduler.Current instead of TaskScheduler.Default in Task.Factory.StartNew calls to maintain flexibility for custom schedulers, even though this requires awareness of potential deadlock risks in certain contexts.

Applied to files:

• test/unit/CommonLibHelperTests.cs

🧬 Code graph analysis (2)

test/unit/CommonLibHelperTests.cs (1)

src/CommonLib/Helpers.cs (5)

• Helpers (17-343)
• DomainNameToDistinguishedName (158-160)
• ConvertSidToHexSid (109-116)
• IsSidFiltered (248-259)
• ConvertLdapTimeToLong (226-232)

test/unit/CommonLibTests.cs (3)

src/CommonLib/Logging/Logging.cs (2)

• Logging (7-20)
• ILogger (31-34)

src/CommonLib/CommonLib.cs (2)

• CommonLib (5-46)
• InitializeCommonLib (15-36)

src/CommonLib/Cache.cs (1)

• SetCacheInstance (151-155)

⏰ 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). (1)

• GitHub Check: build

🔇 Additional comments (8)

test/unit/CommonLibTests.cs (4)

13-16: LGTM!

Constructor properly ensures test isolation by resetting state before each test.

---

18-32: LGTM!

Test correctly verifies that InitializeCommonLib creates a new cache instance and populates all expected internal dictionaries when no cache is provided.

---

34-45: LGTM!

Test correctly verifies that the provided cache instance is used (reference equality check with Assert.Same).

---

47-72: LGTM!

Test correctly verifies that a second initialization attempt logs a warning and does not replace the existing cache. The Moq verification pattern for ILogger is appropriate.

test/unit/CommonLibHelperTests.cs (4)

2-2: LGTM!

Import is necessary for the new SecurityIdentifier usage in the SID conversion tests.

---

329-333: LGTM!

Test correctly verifies that an invalid SID throws an ArgumentException.

---

335-359: LGTM!

The parameterized tests for IsSidFiltered provide good coverage:

• Case-insensitive matching for well-known SIDs
• Prefix-based filtering (S-1-5-80, S-1-5-82, S-1-5-90, S-1-5-96)
• Cases where filtering should return false

Good use of [Theory] and [InlineData] for parameterized testing.

---

361-377: LGTM!

The three tests provide complete coverage of ConvertLdapTimeToLong:

• Null input handling (returns -1)
• Invalid format handling (throws FormatException)
• Valid parsing

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5b982f3 and 80da004.

📒 Files selected for processing (1)

• test/unit/CommonLibTests.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

test/unit/CommonLibTests.cs (3)

src/CommonLib/Logging/Logging.cs (2)

• Logging (7-20)
• ILogger (31-34)

src/CommonLib/CommonLib.cs (2)

• CommonLib (5-46)
• InitializeCommonLib (15-36)

src/CommonLib/Cache.cs (1)

• SetCacheInstance (151-155)

⏰ 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). (1)

• GitHub Check: build

🔇 Additional comments (5)

test/unit/CommonLibTests.cs (5)

18-32: LGTM!

The test correctly verifies that the first initialization without a provided cache creates and populates the Cache singleton with all expected dictionaries.

---

34-45: LGTM!

The test correctly verifies that when a cache instance is provided, that exact instance becomes the active cache. Using Assert.Same is appropriate for reference equality.

---

63-71: LGTM!

The mock verification correctly validates that the warning is logged when attempting to initialize CommonLib a second time. The use of It.IsAnyType pattern is appropriate for verifying structured logging with ILogger.

---

74-88: LGTM!

The helper method correctly uses reflection to reset CommonLib's private static state for test isolation. The error handling for the missing field is appropriate, and the cache reset is intentional.

---

13-16: Consider test isolation with static state.

The constructor resets static CommonLib state before each test, which is good for isolation. However, since these tests manipulate shared static fields (_initialized and Cache singleton), parallel execution with other test classes that also test CommonLib could lead to race conditions or test interference.

Run the following script to check if there are other test classes that initialize CommonLib:

#!/bin/bash
# Search for other test classes that call InitializeCommonLib
rg -n -C3 'InitializeCommonLib' --type=cs -g '!CommonLibTests.cs'

If other test classes exist, consider using xUnit's [Collection] attribute to ensure these tests run sequentially.

[coderabbitai]

Actionable comments posted: 4

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 74064cf and 0609651.

📒 Files selected for processing (3)

• test/unit/CollectionDefinitions/CacheTestsCollectionDefinition.cs (1 hunks)
• test/unit/CommonLibTests.cs (1 hunks)
• test/unit/UserRightsAssignmentProcessorTest.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• test/unit/CommonLibTests.cs

🧰 Additional context used

🧬 Code graph analysis (1)

test/unit/UserRightsAssignmentProcessorTest.cs (4)

src/CommonLib/Cache.cs (1)

• SetCacheInstance (151-155)

test/unit/Facades/MockLdapUtils.cs (2)

• MockLdapUtils (27-30)
• Group (1099-1104)

test/unit/Facades/Consts.cs (1)

• Consts (3-8)

src/CommonLib/OutputTypes/LocalGroup.cs (1)

• LocalGroup (3-8)

⏰ 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). (1)

• GitHub Check: build

🔇 Additional comments (2)

test/unit/UserRightsAssignmentProcessorTest.cs (1)

20-20: Good cache isolation practice.

The Collection attribute correctly wires this test class into the non-parallelizable collection, ensuring proper isolation for tests that manipulate the shared cache singleton.

test/unit/CollectionDefinitions/CacheTestsCollectionDefinition.cs (1)

8-9: Correct approach for cache test isolation.

The collection definition with DisableParallelization = true is the correct approach for tests that manipulate shared cache state. The empty class body is standard for xUnit collection definitions.