Fixup several warnings #9009

plemarquand · 2025-08-11T14:10:04Z

Remove unnecessary try statements for function calls that do not throw
Remove unnecessary #expect checks for values that cannot be nil

- Remove unnecessary `try` statements for function calls that do not throw - Remove unnecessary `#expect` checks for values that cannot be `nil`

plemarquand · 2025-08-11T15:03:39Z

@swift-ci test

plemarquand · 2025-08-12T20:08:19Z

@swift-ci test

plemarquand · 2025-08-13T17:47:00Z

@swift-ci test

dschaefer2 · 2025-08-13T18:40:53Z

@swift-ci please test windows

bkhouri

Thanks for the cleanup. I have some comments, which some possibly being blocking.

bkhouri · 2025-08-13T22:01:28Z

Sources/_InternalTestSupport/GitRepositoryExtensions.swift

@@ -13,6 +13,7 @@
 import SourceControl

 import class Basics.AsyncProcess
+import class TSCBasic.Process


suggestion: can we use AsyncProcess that is located in SwiftPM instead of having to rely on a TSC API?

Switching from Process.checkNonZeroExit to AsyncProcess.checkNonZeroExit would require many methods to be converted to async. While probably worthwhile, it would would be best addressed in a standalone PR.

bkhouri · 2025-08-13T22:02:32Z

Sources/_InternalTestSupport/misc.swift

@@ -193,7 +193,6 @@ public enum TestError: Error {
    case platformNotSupported
 }

-@available(*, deprecated, message: "Migrate test to Swift Testing and use 'fixture' instead")


issue: Please reading this as we want to to encourage contributor to migrate away from XCTest when they touch a test file.

It's adding quite a bit of noise to test builds, but I can add it back.

bkhouri · 2025-08-13T22:03:04Z

Sources/_InternalTestSupport/misc.swift

@@ -32,12 +32,12 @@ import Testing
 import func XCTest.XCTFail
 import struct XCTest.XCTSkip

+import class TSCBasic.Process


issue: Can we use AsyncProcess located in SwiftPM instead of depending on a TSC API.

That would require marking the utility method as async and then cascading that across all its usages. Probably worth doing, but in a separate patch

bkhouri · 2025-08-13T22:04:16Z

Tests/CommandsTests/PackageCommandTests.swift

-        for case let url as URL in enumerator where url.lastPathComponent == "Bar.symbols.json" {
-            symbolGraphURL = url
-            break
+        while let object = enumerator.nextObject() {


can we revert changes to this file as #9012 migrate this suite to SwiftTesting, and may address this.

bkhouri · 2025-08-13T22:05:14Z

Tests/FunctionalTests/MiscellaneousTests.swift

@@ -686,7 +686,7 @@ final class MiscellaneousTestCase: XCTestCase {
                guard case SwiftPMError.executionFailure(_, _, let stderr) = error else {
                    return XCTFail("invalid error \(error)")
                }
-                XCTAssert(stderr.contains("error: You don’t have permission"), "expected permissions error. stderr: '\(stderr)'")
+                XCTAssert(stderr.contains("error: You don’t have permission") || stderr.contains("invalid access"), "expected permissions error. stderr: '\(stderr)'")


issue (possibly-blocking): I this is also update a test case. do we know if this new expectation is expected, or whether it's a regression?

bkhouri · 2025-08-13T22:05:21Z

Tests/FunctionalTests/MiscellaneousTests.swift

@@ -721,7 +721,7 @@ final class MiscellaneousTestCase: XCTestCase {
                guard case SwiftPMError.executionFailure(_, _, let stderr) = error else {
                    return XCTFail("invalid error \(error)")
                }
-                XCTAssert(stderr.contains("error: You don’t have permission"), "expected permissions error. stderr: '\(stderr)'")
+                XCTAssert(stderr.contains("error: You don’t have permission") || stderr.contains("invalid access"), "expected permissions error. stderr: '\(stderr)'")


issue (possibly-blocking): I this is also update a test case. do we know if this new expectation is expected, or whether it's a regression?

This change is unrelated to my patch, but its because this test is failing on main. I had to fix it to get CI to pass. To verify, make sure you do a swift package update first.

bkhouri · 2025-08-13T22:05:27Z

Tests/FunctionalTests/MiscellaneousTests.swift

@@ -756,7 +756,7 @@ final class MiscellaneousTestCase: XCTestCase {
                guard case SwiftPMError.executionFailure(_, _, let stderr) = error else {
                    return XCTFail("invalid error \(error)")
                }
-                XCTAssert(stderr.contains("error: You don’t have permission"), "expected permissions error. stderr: '\(stderr)'")
+                XCTAssert(stderr.contains("error: You don’t have permission") || stderr.contains("invalid access"), "expected permissions error. stderr: '\(stderr)'")


issue (possibly-blocking): I this is also update a test case. do we know if this new expectation is expected, or whether it's a regression?

bkhouri · 2025-08-13T22:21:26Z

@swift-ci test self hosted

plemarquand requested review from jakepetroules, dschaefer2, shawnhyam, bripeticca, owenv, bkhouri, cmcgee1024 and daveyc123 as code owners August 11, 2025 14:10

Fixup several warnings

df13ad4

- Remove unnecessary `try` statements for function calls that do not throw - Remove unnecessary `#expect` checks for values that cannot be `nil`

plemarquand force-pushed the fixup-warnings branch from 6625710 to df13ad4 Compare August 11, 2025 15:03

jakepetroules approved these changes Aug 11, 2025

View reviewed changes

Move off deprecated function

4855cdd

plemarquand force-pushed the fixup-warnings branch from 3e6cd06 to 4855cdd Compare August 12, 2025 20:07

Attempt to fix failing tests (unrelated?)

a006a63

dschaefer2 approved these changes Aug 13, 2025

View reviewed changes

bkhouri reviewed Aug 13, 2025

View reviewed changes

Fixup several warnings #9009

Are you sure you want to change the base?

Fixup several warnings #9009

Conversation

plemarquand commented Aug 11, 2025

Uh oh!

plemarquand commented Aug 11, 2025

Uh oh!

plemarquand commented Aug 12, 2025

Uh oh!

plemarquand commented Aug 13, 2025

Uh oh!

dschaefer2 commented Aug 13, 2025

Uh oh!

bkhouri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkhouri commented Aug 13, 2025

Uh oh!

Uh oh!