-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[test] Fix testExamplePackageDealer #9134
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.
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
[test] Fix testExamplePackageDealer #9134
Conversation
@swift-ci test |
I removed the |
@swift-ci test |
thank you @jakepetroules for having approved. The test was still failing on the CI because the I made two changes |
@bkhouri does this make sense? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I question the purpose and intension of this test and whether we have coverage already via other SwiftPM tests.
If the intent is to ensure the project builds, this should be handled by a https://github.com/apple/example-package-dealer PR build.
|
||
#if os(macOS) | ||
// On macOS, we add the HOME variable to avoid git errors. | ||
try sh("git\(ProcessInfo.exeSuffix)", "clone", "https://github.com/apple/example-package-dealer", packagePath, env: ["HOME": tempDir.pathString]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: what git error occur on macOS? can we also add the same for all platform to ensure the test is uniform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command failed with exit code: terminated(code: 1) - command: git clone https://github.com/apple/example-package-dealer /var/folders/14/nwpsn4b504gfp02_mrbyd2jr0000gr/T/TemporaryDirectory.Wew2EB/dealer
stdout:
stderr:
Cloning into '/var/folders/14/nwpsn4b504gfp02_mrbyd2jr0000gr/T/TemporaryDirectory.Wew2EB/dealer'...
Could not get home directory: $HOME is not defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only test I found that uses the git
command via a shell.
The other tests using git are using SPM's GitRepositoryProvider
struct from
public struct GitRepositoryProvider: RepositoryProvider, Cancellable { |
|
||
// Do not run the test when the git clone operation failed | ||
if !FileManager.default.fileExists(atPath: packagePath.pathString) { | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fond of this as it gives the impression the tests passes, when it wasn't executed at all. We should include a TODO to use the Test cancellation feature once it's available.
https://forums.swift.org/t/pitch-test-cancellation/81847/18
In the meantime, can we record a warning Issue.record("....", severity: .warning)
as introduced by https://forums.swift.org/t/accepted-st-0013-test-issue-severity/81385/2 (though our self-hosted pipelines doesn't currently use a version of the toolchain that has these features)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this code to record the issue, only when compiling with Swift 6.3 or more recent
#if compiler(>=6.3)
Issue.record("Can't clone the repository \(repoToClone), abording the test.", severity: .warning)
#endif
@bkhouri I let you judge if this test is useful or not. I don't have the overall view of all test. I was just trying to fix that one that fails on our CI when building the Swift toolchain. I was surprised it passes on Swift.org's CI If you deem this is not necessary anymore, happy to delete it entirely :-) |
@swift-ci test |
@sebsto: We can delete this test once https://github.com/swiftlang/example-package-dealer/ does not have any PR builds. @shahmishal : where should I file an issue for this as Issues are not enabled in the |
@swift-ci test windows |
@bkhouri It seems the errors we see on Windows are not related to this change
|
@bkhouri any blocker left before merging this ? |
This is now merged: swiftlang/example-package-dealer#21 |
No blockers from my perspective. |
This is a known issue: #8602 |
The test
testExamplePackageDealer
was failing on Amazon Linux and not in Swift.org's CISee #9126
Motivation:
The reason why the test is working on Swift.org's CI is because it silently ignores outbound connection issues. The test fails to clone the project on github.com and silently stops.
When the clone operation succeed, the test fails. This is the case on macOS and Amazon Linux 2023, probably others OSes too.
Modifications:
I observed multiple problems in this test.
the test was not marked
async
and thewithTemporaryDirectory
was not await. The test completed execution before thegit clone
operation finished.The
git clone
operation requires an environment variableHOME
to be definedOn Swift 6.1 and later, the second build recompiles the plugins. There are some lines
Compiling plugin
that are valid and should not fail the test.Here is an example of a valid output. It contains
Compiling plugin
Result:
The test passes on macOS and Amazon Linux 2023