Repository navigation
docs: fix TESTING.md accuracy — unit tests do touch some OS APIs - #412
Merged
Merged
Conversation
- TEST-M3: Corrected claim that unit tests are pure (no OS access). Some tests access registry, process list, and Task Scheduler. - Updated UITests row to reflect CI execution (headless runner).
📝 WalkthroughWalkthroughTESTING.md is updated to document which test projects run on CI versus locally. Unit tests now run on CI with a note about potential lightweight OS API usage, UI tests now run on CI headlessly (limited), and integration tests remain local-only. ChangesTesting Documentation Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@TESTING.md`:
- Line 9: The README line for SysManager.Tests overstates OS API usage; update
the documentation so it only lists APIs actually invoked: remove "Registry
reads" and "Process enumeration" and replace with a note that tests call
Process.GetCurrentProcess(), and clarify that Task Scheduler is only referenced
via the StartupSource.TaskScheduler enum in test data (no Task Scheduler API
calls). Edit the table entry for `SysManager.Tests` to reflect these exact
symbols (Process.GetCurrentProcess, StartupSource.TaskScheduler) and remove any
claim of registry or broader process enumeration usage.
- Line 11: Update the TESTING.md table entry for SysManager.UITests to remove
the incorrect "headless" descriptor: locate the row containing
`SysManager.UITests` and change the status cell from "✅ CI (headless, limited)"
to something that reflects a full desktop session (e.g. "✅ CI (desktop session,
limited)" or simply "✅ CI (limited)"), ensuring "limited" is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 569c7a7d-2431-4d16-a018-48fa2ad464a0
📒 Files selected for processing (1)
TESTING.md
📜 Review details
⏰ 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). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
| | Project | What it tests | Runs on CI | | ||
| |---|---|---| | ||
| | `SysManager.Tests` | Pure unit tests — no WPF, no WMI, no network, no process spawning | ✅ Every push / PR | | ||
| | `SysManager.Tests` | Unit tests — mostly pure logic, but some tests touch lightweight OS APIs (registry reads, process enumeration, Task Scheduler queries). No WPF dispatcher, no WMI, no network I/O, no admin required. | ✅ Every push / PR | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that SysManager.Tests actually uses the OS APIs mentioned in the documentation.
echo "=== Searching for Registry API usage in SysManager.Tests ==="
rg -n --type=cs -C3 'Registry\.' --glob '**/SysManager.Tests/**' --glob '!**/obj/**' --glob '!**/bin/**'
echo -e "\n=== Searching for Process enumeration API usage ==="
rg -n --type=cs -C3 'Process\.(GetProcesses|GetProcessById|GetCurrentProcess)' --glob '**/SysManager.Tests/**' --glob '!**/obj/**' --glob '!**/bin/**'
echo -e "\n=== Searching for Task Scheduler API usage ==="
rg -n --type=cs -C3 'TaskScheduler|TaskService' --glob '**/SysManager.Tests/**' --glob '!**/obj/**' --glob '!**/bin/**'
echo -e "\n=== Verifying absence of WMI, WPF dispatcher usage in unit tests ==="
rg -n --type=cs 'ManagementObjectSearcher|Dispatcher\.(Invoke|BeginInvoke)' --glob '**/SysManager.Tests/**' --glob '!**/obj/**' --glob '!**/bin/**' || echo "✓ No WMI or WPF dispatcher usage found (as expected)"Repository: laurentiu021/SystemManager
Length of output: 4862
Documentation claims about OS API usage in SysManager.Tests are inaccurate and need correction.
Verification of the claimed OS API usage shows:
- Registry reads: Not found in test code (claimed but absent)
- Process enumeration: Only
Process.GetCurrentProcess()found; no broader process enumeration likeGetProcesses() - Task Scheduler queries: Tests reference
StartupSource.TaskSchedulerenum values in test data but do not actually call Task Scheduler APIs
The documentation should be updated to accurately reflect only the OS APIs actually invoked in the tests, not the broader scope currently claimed.
🤖 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 `@TESTING.md` at line 9, The README line for SysManager.Tests overstates OS API
usage; update the documentation so it only lists APIs actually invoked: remove
"Registry reads" and "Process enumeration" and replace with a note that tests
call Process.GetCurrentProcess(), and clarify that Task Scheduler is only
referenced via the StartupSource.TaskScheduler enum in test data (no Task
Scheduler API calls). Edit the table entry for `SysManager.Tests` to reflect
these exact symbols (Process.GetCurrentProcess, StartupSource.TaskScheduler) and
remove any claim of registry or broader process enumeration usage.
| | `SysManager.Tests` | Unit tests — mostly pure logic, but some tests touch lightweight OS APIs (registry reads, process enumeration, Task Scheduler queries). No WPF dispatcher, no WMI, no network I/O, no admin required. | ✅ Every push / PR | | ||
| | `SysManager.IntegrationTests` | Integration tests — real Windows APIs (Event Log, WMI, PowerShell, ICMP, WPF dispatcher) | ❌ Local only | | ||
| | `SysManager.UITests` | End-to-end UI automation via FlaUI | ❌ Local only (needs a desktop session) | | ||
| | `SysManager.UITests` | End-to-end UI automation via FlaUI | ✅ CI (headless, limited) | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that CI configuration actually runs SysManager.UITests.
echo "=== Searching for CI workflow files ==="
fd -e yml -e yaml . .github/workflows 2>/dev/null || echo "No .github/workflows directory found"
echo -e "\n=== Checking for UITests in CI configuration ==="
rg -n 'SysManager\.UITests' --glob '**/.github/workflows/*.yml' --glob '**/.github/workflows/*.yaml' -C5 || echo "No UITests reference found in workflow files"
echo -e "\n=== Checking for headless execution settings ==="
rg -n 'headless|HEADLESS|Xvfb' --glob '**/.github/workflows/*.yml' --glob '**/.github/workflows/*.yaml' -C3 || echo "No explicit headless configuration found"Repository: laurentiu021/SystemManager
Length of output: 731
🏁 Script executed:
#!/bin/bash
# Read the main CI workflow file
echo "=== Reading .github/workflows/ci.yml ==="
cat .github/workflows/ci.yml
echo -e "\n=== Reading TESTING.md around line 11 ==="
cat -n TESTING.md | head -20Repository: laurentiu021/SystemManager
Length of output: 4829
Remove "headless" descriptor — UITests run on full Windows desktop session, not headless.
The documentation correctly states that SysManager.UITests runs on CI, but the "headless" descriptor is inaccurate. The CI workflow explicitly runs on windows-latest, which provides a full desktop session for UI automation. The ci.yml workflow confirms this with the comment: "UI tests need a desktop session; windows-latest provides one." The "limited" qualifier is accurate (tests only run on push and PRs from the same repository, not fork PRs).
🤖 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 `@TESTING.md` at line 11, Update the TESTING.md table entry for
SysManager.UITests to remove the incorrect "headless" descriptor: locate the row
containing `SysManager.UITests` and change the status cell from "✅ CI (headless,
limited)" to something that reflects a full desktop session (e.g. "✅ CI (desktop
session, limited)" or simply "✅ CI (limited)"), ensuring "limited" is preserved.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
…y-catch) (#462) ## Summary Resolves the last 5 fixable CodeQL alerts in the codebase: | Alert | Rule | File | Fix | |-------|------|------|-----| | #509 | cs/linq/missed-where | UninstallerService.cs | Replaced foreach+ContainsKey with TryAdd | | #508 | cs/linq/missed-select | NetworkSharedState.cs | Converted foreach+map to .Select().ToList() | | #507 | cs/linq/missed-select | IconExtractorService.cs | Converted foreach+map to .Select().FirstOrDefault() | | #412 | cs/linq/missed-select | IconExtractorService.cs | Converted foreach+IndexOf to LINQ pipeline | | #445 | cs/empty-catch-block | StartupService.cs | Added Log.Debug to empty catch | ## Remaining CodeQL alerts All remaining alerts are either: - P/Invoke / unmanaged code declarations (acceptable, required for Win32 interop) - Auto-generated code in obj/ (not our code) ## Testing - Build: 0 errors - No behavioral changes (pure style refactoring) - CI will validate Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
laurentiu021
added a commit
that referenced
this pull request
May 22, 2026
- TEST-M3: Corrected claim that unit tests are pure (no OS access). Some tests access registry, process list, and Task Scheduler. - Updated UITests row to reflect CI execution (headless runner). Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes TEST-M3 finding: TESTING.md incorrectly claimed unit tests are pure (no OS access). Some tests in SysManager.Tests do access registry, process enumeration, and Task Scheduler.
Changes
Files changed (1)
TESTING.md
Build
docs: commit — no code changes.