Skip docker tests when docker is not running - #1412
Skip docker tests when docker is not running#1412Rodney Richardson (RodneyRichardson) wants to merge 8 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. |
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, []);
| await this.SkipIfDockerNotRunningAsync(); | ||
|
|
||
| // CanPingDockerAsync should return true or false if docker is running | ||
| await this.dockerService.CanRunLinuxContainersAsync(); |
| await this.SkipIfDockerNotRunningAsync(); | ||
|
|
||
| await this.dockerService.TryPullImageAsync(LinuxTestImageWithBaseDetails); | ||
| var details = await this.dockerService.InspectImageAsync(LinuxTestImageWithBaseDetails); |
Fixes #1411