Cleanup SPMTestSupport Module #3019

mattt · 2020-11-04T14:01:16Z

One last incremental PR in service of #2967, this time doing some clean up in the SPMTestSupport module.

In my work to update the semantics of Swift package identity, I've spent a lot of time in Workspace-adjacent tests. And one of the things that's been tripping me up is the use of Test___ as a prefix for supporting types, like TestTarget and TestProduct, which I often conflate with .testTarget or the like. I think the prefix Mock___ provides clearer, more descriptive names for these types.

Along the way, I also fixed some unused or misnamed APIs that I encountered. For example, there was an unused MockDependencyGraph type in addition to a MockGraph type that was used, so I renamed the latter to the former (2f283f0).

As always, most of these changes are severable from one another and can be picked or dropped a la carte.

Edit: Any incidental formatting or whitespace changes are from SwiftFormat (#3012), which I have set up to run automatically in a pre-commit hook (.git/hooks/pre-commit) for each staged file:

#!/bin/bash
git-format-staged --formatter "swiftformat stdin --stdinpath '{}'" "*.swift"

Rename TestWorkspaceDelegate to MockWorkspaceDelegate

…hat name

…tension

…hat name

Remove MockLoadingError type Rename MockDependencyResolver.swift to MockPackageContainer.swift

mattt · 2020-11-04T20:52:16Z

@swift-ci please smoke test

Sources/SPMTestSupport/MockWorkspace.swift

abertelrud · 2020-11-04T18:50:10Z

Sources/SPMTestSupport/MockWorkspace.swift

@@ -61,48 +59,48 @@ public final class TestWorkspace {
        self.skipUpdate = skipUpdate
        self.enablePubGrub = enablePubGrub

-        try create()
+        try self.create()


Are all these self. additions just stylistic? I think the general Swift approach (and the one used in SwiftPM) is to not specify self. when it's not needed (such as in an escaping closure).

These were automatically inserted by SwiftFormat, for which @tomerd added the configuration with #3012. I believe we share stylistic preferences, but I'm more than happy to accede to any conventions enforced by a formatter.

That said, I've encountered a few situations in which SwiftFormat makes source-breaking changes to Swift Package Manager. Namely, mistakenly inserting self. for things like break labels and local variables whose names shadow an instance method or property. To reproduce this, try running swiftformat with the current settings on Sources/Workspace/Workspace.swift.

I'm working to share minimal examples with the maintainer, but this may be cause for reconsidering how automatic formatting is currently set up.

Are all these self. additions just stylistic? I think the general Swift approach (and the one used in SwiftPM) is to not specify self. when it's not needed (such as in an escaping closure).

These were automatically inserted by SwiftFormat, for which @tomerd added the configuration with #3012. I believe we share stylistic preferences, but I'm more than happy to accede to any conventions enforced by a formatter.

In other projects (SwiftNIO, etc) we actually encouraged adding self. to make sure things are very explicit. happy to open up for discussion tho. cc @weissi

That said, I've encountered a few situations in which SwiftFormat makes source-breaking changes to Swift Package Manager. Namely, mistakenly inserting self. for things like break labels and local variables whose names shadow an instance method or property.

interesting. I have not had such trouble with SwiftFormat in other projects, what version are you using @mattt?

@mattt in any case dont let this block progress. you can run the formatter without the self setting if it breaks things and we can revisit this separately

@tomerd According to swiftformat --version, I'm using the latest (0.47.2). For now, I'm committing with --no-verify to skip my pre-commit hook as necessary. So this isn't a blocker.

This is fine, it's just opposite of what I've seen before so I wanted to ask.

Sources/SPMTestSupport/MockWorkspace.swift

abertelrud · 2020-11-04T18:52:04Z

Sources/SPMTestSupport/XCTAssertHelpers.swift

+public func XCTAssertEqual<T: CustomStringConvertible>(
+    _ assignment: [(container: T, version: Version)],
+    _ expected: [T: Version],
+    file: StaticString = #file, line: UInt = #line


Since this code is already being moved around now, should this be switched to #fileId to avoid future warnings about #fileId vs #filePath (once they come back)?

In the interest of keeping this focused, let's punt on that. For something like #file usage, I think we'll want to change everything project-wide at once, rather than adopting piecemeal over time.

Sounds good.

abertelrud · 2020-11-04T18:54:39Z

Tests/XCBuildSupportTests/PIFBuilderTests.swift

@@ -138,7 +138,7 @@ class PIFBuilderTests: XCTestCase {
                    v: .v5_2,
                    packageKind: .remote,
                    products: [
-                        .init(name: "BarLib", targets: ["BarLib"]),
+                        .init(name: "BarLib", type: .library(.automatic), targets: ["BarLib"]),


Was this a needed change? The default type should be automatic. Or is the intent that for the PIF, the type is always spelled out?

This is a consequence of a3a67f9, which removes a helper overload that implies the type parameter. I found the ambiguity at the call site not to be worth the benefit of the affordance. If it makes sense for this to be the default argument, I think that change should be made in the original declaration.

Thanks for clarifying.

abertelrud · 2020-11-04T23:04:47Z

Thanks for all the continuing cleanup, @mattt! I agree about the change from Test to Mock, but just had a couple of questions about the other changes.

mattt · 2020-11-05T19:35:49Z

@abertelrud Thanks so much for reviewing this. I just finished responding to all of your feedback. Let me know if there's anything else you'd like to discuss before merging this in.

abertelrud · 2020-11-07T00:06:56Z

Thanks, this looks great. FYI, I also have a separate change to make SPMTestSupport able to be used for libSwiftPMDataModel, but we can merge yours first and I can rebase on that.

abertelrud

Is this ready to merge?

abertelrud · 2020-11-07T00:08:01Z

Actually, based on your previous comment it sounds as if it is, so I'll merge.

mattt added 21 commits November 4, 2020 04:15

Extract TestWorkspace related types into separate files

6ae224a

Remove unnecessary import statements

98467a0

Rename TestWorkspace to MockWorkspace

3d6cdd9

Rename TestWorkspaceDelegate to MockWorkspaceDelegate

Rename TestTarget to MockTarget

24d3ad7

Rename TestProduct to MockProduct

d9f3598

Rename TestDependency to MockDependency, replacing existing type of t…

5316444

…hat name

Rename TestPackage to MockPackage, replacing existing type of that name

e5e5d09

Remove unused property in Destination extension

f325695

Remove unused constant globalSymbolInMainBinary

3e48bc0

Move XCTAssertEqual overload to XCTAssertHelpers

a9ec7f6

Remove redundant import statement

b999b76

Remove unused type MockPackageContainer2

f7d232d

Rename MockPackagesProvider to MockPackageContainerProvider

215d81b

Remove unused resolveToVersion helper method in DependencyResolver ex…

1a94f0e

…tension

Rename MockGraph to MockDependencyGraph, replacing existing type of t…

2f283f0

…hat name

Extract MockDependencyGraph to separate file

17293e6

Remove getContainer helper method overload

0bf1885

Remove ProductDescription helper initializer overload

a3a67f9

Consolidate Manifest extensions

2ab1f34

Rename Manifest.swift to ManifestExtensions.swift

07bb91e

Rename MockPackageConstraint to MockPackageContainer.Constraint

4cf093e

Remove MockLoadingError type Rename MockDependencyResolver.swift to MockPackageContainer.swift

mattt requested review from abertelrud, aciidgh, friedbunny and neonichu as code owners November 4, 2020 14:01

This comment has been minimized.

Sign in to view

Replace use of MockWorkspace.PackageDependency with MockDependency

041bb61

abertelrud reviewed Nov 4, 2020

View reviewed changes

tomerd assigned neonichu Nov 5, 2020

mattt mentioned this pull request Nov 5, 2020

add swiftformat config file #3012

Merged

abertelrud self-requested a review November 7, 2020 00:07

abertelrud approved these changes Nov 7, 2020

View reviewed changes

abertelrud merged commit b565edc into swiftlang:main Nov 7, 2020

mattt deleted the spmtestsupport-module-cleanup branch November 7, 2020 12:20

abertelrud mentioned this pull request Nov 9, 2020

Fix a build failure resulting from a race condition between two PRs. #3038

Merged

Cleanup SPMTestSupport Module #3019

Cleanup SPMTestSupport Module #3019

Uh oh!

Conversation

mattt commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

mattt commented Nov 4, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattt Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomerd Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

abertelrud commented Nov 4, 2020

Uh oh!

mattt commented Nov 5, 2020

Uh oh!

abertelrud commented Nov 7, 2020

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Nov 7, 2020

Uh oh!

Uh oh!

mattt commented Nov 4, 2020 •

edited

Loading

mattt Nov 5, 2020 •

edited

Loading

tomerd Nov 5, 2020 •

edited

Loading