chore: resolve xunit warnings BED-8243#303
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughSeven unit test files are refactored to improve test infrastructure and conform to xUnit best practices. Changes include converting async void test methods to async Task, correcting assertion parameter order from (actual, expected) to (expected, actual), replacing blocking synchronization with async patterns, improving assertion expressions, and removing unused dependencies. ChangesTest Best Practices
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/LDAPFilterTest.cs (1)
89-106: ⚡ Quick winConsider materializing
filtersto avoid multiple enumerations.
filtersisIEnumerable<string>and is enumerated four times: once in theforeach(lines 95–100), twice in the twoAssert.Singlecalls (lines 103–104), and once infilters.Count()(line 106). IfGetFilterList()returns a deferred/lazy sequence, the query is re-evaluated each time. Materializing up front removes the ambiguity:♻️ Proposed refactor
- IEnumerable<string> filters = test.GetFilterList(); + IReadOnlyList<string> filters = test.GetFilterList().ToList();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/LDAPFilterTest.cs` around lines 89 - 106, The test re-enumerates the deferred IEnumerable<string> returned by test.GetFilterList() multiple times (in the foreach, two Assert.Single calls, and Count()), which can re-evaluate the sequence or produce different results; materialize the sequence immediately by assigning filters = test.GetFilterList().ToList() (ensure System.Linq is available) and then run the foreach, Assert.Single checks (using the List), and Assert.Equal(2, filters.Count) against the concrete list to avoid multiple enumerations and side effects; references: test.GetFilterList(), variable filters, mandatoryFilter1, mandatoryFilter2, userFilter, computerFilter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit/LDAPFilterTest.cs`:
- Around line 89-106: The test re-enumerates the deferred IEnumerable<string>
returned by test.GetFilterList() multiple times (in the foreach, two
Assert.Single calls, and Count()), which can re-evaluate the sequence or produce
different results; materialize the sequence immediately by assigning filters =
test.GetFilterList().ToList() (ensure System.Linq is available) and then run the
foreach, Assert.Single checks (using the List), and Assert.Equal(2,
filters.Count) against the concrete list to avoid multiple enumerations and side
effects; references: test.GetFilterList(), variable filters, mandatoryFilter1,
mandatoryFilter2, userFilter, computerFilter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc84b48b-ac84-4f72-8a7e-ff05090782f0
📒 Files selected for processing (7)
test/unit/ACLProcessorTest.cstest/unit/DefaultLabelValuesCacheTests.cstest/unit/LDAPFilterTest.cstest/unit/LDAPUtilsTest.cstest/unit/LdapPropertyTests.cstest/unit/MetricAggregatorTests.cstest/unit/SPNProcessorsTest.cs
2 participants
Description
xunit is warning over some syntax issues in tests, this pr updates the tests to resolve the warnings and clean up build output.
Motivation and Context
This PR addresses: BED-8243
How Has This Been Tested?
Ran
dotnet buildanddotnet testScreenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit