Skip docker tests when docker is not running - #1412
Skip docker tests when docker is not running#1412Rodney Richardson (RodneyRichardson) wants to merge 13 commits into
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1412 +/- ##
=====================================
Coverage 89.7% 89.7%
=====================================
Files 407 407
Lines 32294 32294
Branches 1990 1990
=====================================
+ Hits 28990 28991 +1
Misses 2879 2879
+ Partials 425 424 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The DockerService_CanPingDockerAsync test requires docker to be installed and running on the agent to pass, but are still necessary in cases where there is a windows machine running a docker daemon, if we can instead have this particular test conditionally skip or return an inconclusive when there is no docker instance running that would be better, The other skips are okay |
There was a problem hiding this comment.
Pull request overview
This PR aims to address Windows test failures (Issue #1411) by excluding certain tests from running on Windows via MSTest’s [OSCondition] attribute in the Microsoft.ComponentDetection.Common.Tests test suite.
Changes:
- Added
[OSCondition(ConditionMode.Exclude, OperatingSystems.Windows)]toDockerService_CanPingDockerAsync. - Removed
[TestMethod]annotations from twoSafeFileEnumerableTeststests while leaving[OSCondition(…Exclude, …Windows)]in place.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/Microsoft.ComponentDetection.Common.Tests/SafeFileEnumerableTests.cs | Removes [TestMethod] from two Windows-excluded tests (risk: test discovery may stop entirely). |
| test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs | Excludes the Docker ping test from running on Windows. |
Suppressed comments (1)
test/Microsoft.ComponentDetection.Common.Tests/SafeFileEnumerableTests.cs:113
[TestMethod]was removed, which likely prevents MSTest from discovering/running this test on any OS. Re-add[TestMethod]so the test still runs on non-Windows platforms while being excluded on Windows.
[OSCondition(ConditionMode.Exclude, OperatingSystems.Windows)]
public void GetEnumerator_DuplicatePathIgnored()
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I've tested on my Windows 11 machine when running docker (with WSL2), and when not running docker, and it seems to skip appropriately. Edit: I looks that some tests will still fail on Windows if WSL is not enabled, as looks to be the case in the CI pipeline. More work is needed, so I've marked as draft again. |
Rodney Richardson (RodneyRichardson)
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs:52
- Several Docker integration tests no longer exclude Windows (the prior
[OSCondition(ConditionMode.Exclude, OperatingSystems.Windows)]attributes were removed). These tests still assume Linux-container behavior (e.g.,CanRunLinuxContainersAsync()returns true only when Docker reportsOSType == "linux"), so on Windows with Docker Desktop set to Windows containers they will reliably fail rather than be skipped. This also seems to contradict the PR title/linked issue about skipping failing tests on Windows.
Consider restoring the Windows exclusion (or adding an explicit runtime skip when CanRunLinuxContainersAsync() is false) for all tests that require Linux containers/images: DockerService_CanRunLinuxContainersAsync, DockerService_CanPullImageAsync, DockerService_CanInspectImageAsync, DockerService_PopulatesBaseImageAndLayerDetailsAsync, and DockerService_CanCreateAndRunImageAsync.
[TestMethod]
public async Task DockerService_CanRunLinuxContainersAsync()
{
await this.SkipIfDockerNotRunningAsync();
var isLinuxContainerModeEnabled = await this.dockerService.CanRunLinuxContainersAsync();
isLinuxContainerModeEnabled.Should().BeTrue();
}
test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs:43
DockerService_CanPingDockerAsync_DoesNotThrowcurrently has no assertion; the test will pass as long as the call completes. Since the intent is explicitly “does not throw” (and the file already usesNotThrow()/NotThrowAsync()patterns), it would be clearer and more robust to assert that explicitly.
This issue also appears on line 45 of the same file.
[TestMethod]
public async Task DockerService_CanPingDockerAsync_DoesNotThrow()
{
// CanPingDockerAsync should return true or false, regardless of Operating System
await this.dockerService.CanPingDockerAsync();
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs:113
- This test always creates/runs a Linux container. On Windows hosts running Docker in Windows container mode, this will likely fail. Consider skipping inconclusively when Linux container mode is not available.
await this.SkipIfDockerNotRunningAsync();
var (stdout, stderr) = await this.dockerService.CreateAndRunContainerAsync(LinuxTestImage, []);
There was a problem hiding this comment.
🟢 Approval recommended
The changes are isolated to tests and correctly prevent failures when Docker/Linux-container support isn’t available, with only minor message/comment refinements suggested.
Review details
Suppressed comments (1)
test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs:63
- This inline comment appears to be a copy/paste error: it refers to CanPingDockerAsync, but this test is exercising CanRunLinuxContainersAsync.
// CanRunLinuxContainersAsync should return true or false if docker is running
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
One updated “DoesNotThrow” test now skips the Docker-not-running scenario, reducing coverage of the exact failure mode this PR aims to harden against.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs:64
DockerService_CanRunLinuxContainersAsync_DoesNotThrowcurrently skips when Docker isn't running, which prevents this test from exercising the scenario that should be handled gracefully (return false without throwing). Consider removing the skip so this test actually guards against regressions whereCanRunLinuxContainersAsync()might throw when Docker is unavailable.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Two updated tests no longer assert any meaningful behavior (they effectively always pass), reducing the value of the test suite unless adjusted as suggested.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs:65
DockerService_CanRunLinuxContainersAsync_DoesNotThrowignores the returned value, so as long as Docker is running this test will almost always pass even if Linux containers are unexpectedly unsupported. Consider treating “not supported” as an inconclusive skip and assertingtruewhen Linux containers are available, so the test still validates expected behavior in environments configured for Linux containers.
public async Task DockerService_CanRunLinuxContainersAsync_DoesNotThrow()
{
await this.SkipIfDockerNotRunningAsync();
// CanRunLinuxContainersAsync should return true or false if docker is running
await this.dockerService.CanRunLinuxContainersAsync();
}
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
… docker is not running.
There was a problem hiding this comment.
🟢 Approval recommended
The changes are low-risk and correctly reduce environment-dependent test failures, with only minor message/comment clarity nits.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are limited to tests and should reduce environment-dependent failures; remaining feedback is minor hardening for CI reliability and clearer assertions.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs:49
- The docker availability checks can hang for a long time if Docker is installed but unresponsive (socket/daemon stuck). Adding a short timeout and treating timeout as inconclusive will keep CI from stalling indefinitely.
test/Microsoft.ComponentDetection.Common.Tests/DockerServiceTests.cs:81 - This test ignores the boolean result from TryPullImageAsync; if the pull fails and InspectImageAsync returns null, the failure message will be less direct. Assert the pull result so failures clearly indicate whether the pull step failed.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
Fixes #1411