Skip to content

Fixes many tests (plus skips) to get more tests running on windows #8609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 15, 2025
Merged

Fixes many tests (plus skips) to get more tests running on windows #8609

merged 1 commit into from
May 15, 2025

Conversation

daveinglis
Copy link
Contributor

@daveinglis daveinglis commented May 2, 2025 •
edited
Loading

Now that we can have executable dependencies on test targets for windows this change updates many tests that have not been building/running on windows.

closes #8606
closes #8658
closes #8547

@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch 2 times, most recently from 11e5eb2 to 4948871 Compare May 2, 2025 15:45
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 4948871 to 50d0a9d Compare May 2, 2025 17:04
@daveinglis
Copy link
Contributor Author

@swift-ci test

1 similar comment
@daveinglis
Copy link
Contributor Author

@swift-ci test

bkhouri
bkhouri reviewed May 5, 2025
View reviewed changes
Copy link
Contributor

@bkhouri bkhouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work enabling a bunch of tests on Windows. I know this is a Draft PR, but I have some comments, some of why may not be applicable based on the remaining work prior to marking as Ready :)

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 50d0a9d to afc03c9 Compare May 5, 2025 16:51
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from afc03c9 to c94f13d Compare May 5, 2025 21:07
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch 3 times, most recently from 566280b to 0fb200a Compare May 6, 2025 14:09
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch 2 times, most recently from 49246de to 945420b Compare May 6, 2025 21:08
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 945420b to 09a553d Compare May 7, 2025 13:29
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 09a553d to 8fa313e Compare May 7, 2025 14:50
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 8fa313e to 73a640f Compare May 7, 2025 16:42
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis
Copy link
Contributor Author

@swift-ci test self hosted windows

3 similar comments
@daveinglis
Copy link
Contributor Author

@swift-ci test self hosted windows

@daveinglis
Copy link
Contributor Author

@swift-ci test self hosted windows

@daveinglis
Copy link
Contributor Author

@swift-ci test self hosted windows

@daveinglis
Copy link
Contributor Author

@swift-ci test windows

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 0c774f6 to 0b0a4c2 Compare May 12, 2025 16:23
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis
Copy link
Contributor Author

@swift-ci test windows

1 similar comment
@daveinglis
Copy link
Contributor Author

@swift-ci test windows

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 0b0a4c2 to 947b33e Compare May 13, 2025 00:19
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis
Copy link
Contributor Author

@swift-ci test windows

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 947b33e to 92f918e Compare May 13, 2025 19:14
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis
Copy link
Contributor Author

@swift-ci test windows

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 92f918e to b380a7d Compare May 14, 2025 13:58
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from b380a7d to 0de27ae Compare May 14, 2025 14:07
@daveinglis
Copy link
Contributor Author

@swift-ci test

@bkhouri
Copy link
Contributor

bkhouri commented May 14, 2025

@swift-ci test windows

@daveinglis daveinglis force-pushed the pr/windows_test_fixes branch from 0de27ae to 7d389f3 Compare May 14, 2025 17:54
@daveinglis
Copy link
Contributor Author

@swift-ci test

@daveinglis
Copy link
Contributor Author

@swift-ci test windows

@daveinglis daveinglis marked this pull request as ready for review May 15, 2025 00:34
bkhouri
bkhouri approved these changes May 15, 2025
View reviewed changes
Copy link
Contributor

@bkhouri bkhouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @daveinglis . It's great to see increased test coverage on Windows.

I have some comments, but nothing blocking.

Fixtures/Miscellaneous/PluginGeneratedResources/Package.swift
@@ -1,4 +1,4 @@
// swift-tools-version: 5.7
// swift-tools-version: 6.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (possibly-blocking): Although this seems like a very simple change, I fear this might introduce regression as the swift-tools-version for the test package was set for 5.7 for a reason.

What is the purpose of updating this? Can we revert this back to 5.7 and maybe disable (or mark the test as expected failure)?

Copy link
Contributor Author

@daveinglis daveinglis May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating the plugin to use URL API instead of Path requires this.

Sources/Build/LLBuildProgressTracker.swift
@@ -559,9 +559,14 @@ private struct CommandTaskTracker {
}

private func progressText(of command: SPMLLBuild.Command, targetName: String?) -> String {
#if os(Windows)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): can we define the path separator in a common location so we don't have to redefine it multiple times whenever a test requires it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something to consider maybe if we ever vender in the TSC Path stuff into swift pm to remove that dependency... some day...

Tests/CommandsTests/PackageCommandTests.swift
@@ -1706,6 +1716,8 @@ class PackageCommandTestCase: CommandsBuildProviderTestCase {
}

func testOnlyUseVersionsFromResolvedFileFetchesWithExistingState() async throws {
try XCTSkipOnWindows(because: "error: Package.resolved file is corrupted or malformed, needs investigation")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: can we raise a GitHub issue to fix this test, and reference the issue in the reason?

This comment applies to all skipped tests and can be done in a follow-up PR.

Tests/FunctionalTests/DependencyResolutionTests.swift
@@ -18,12 +18,14 @@ import _InternalTestSupport
import Workspace
import XCTest

import enum TSCUtility.Git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I wonder if we should vender the Git symbol in SwiftPM so we remove a dependency on TSC. I guess it's a discussion for another time.

@bkhouri bkhouri merged commit 2e98598 into swiftlang:main May 15, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakepetroules jakepetroules jakepetroules left review comments

@bkhouri bkhouri bkhouri approved these changes

@dschaefer2 dschaefer2 Awaiting requested review from dschaefer2 dschaefer2 is a code owner

@shawnhyam shawnhyam Awaiting requested review from shawnhyam shawnhyam is a code owner

@bripeticca bripeticca Awaiting requested review from bripeticca bripeticca is a code owner

@plemarquand plemarquand Awaiting requested review from plemarquand plemarquand is a code owner

@owenv owenv Awaiting requested review from owenv owenv is a code owner

@cmcgee1024 cmcgee1024 Awaiting requested review from cmcgee1024 cmcgee1024 is a code owner

@daveyc123 daveyc123 Awaiting requested review from daveyc123 daveyc123 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@daveinglis @bkhouri @jakepetroules