Add refactoring actions for adding a test targets and products to a package manifest #1193

DougGregor · 2024-04-20T00:44:42Z

Leverage the newly-introduced package manifest editing tools in SwiftPM
to create a package editing refactoring operation. This operation can
be triggered from the a target in the manifest itself, e.g.,

.target(name: "MyLib")

and will add a test target to the package manifest that depends on
this target, i.e.,

.testTarget(
  name: "MyLibTests",
  dependencies: [ "MyLib" ]
)

It will also create a new source file Tests/MyLibTests/MyLibTests.swift
that that imports both MyLib and XCTest, and contains an XCTestCase subclass
with one test to get you started.

DougGregor · 2024-04-20T00:45:14Z

This depends on the SwiftPM changes in swiftlang/swift-package-manager#7474

DougGregor · 2024-04-20T00:45:28Z

swiftlang/swift-package-manager#7474

@swift-ci please test

Package.swift

Sources/LanguageServerProtocol/SupportTypes/TextDocumentEdit.swift

ahoppen · 2024-04-20T05:09:28Z

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

Could you add test cases for these refactorings?

Done in 4c1e119

ahoppen · 2024-04-20T05:11:30Z

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

+    var actions = [CodeAction]()
+
+    // If there's a target name, offer to create a test target derived from it.
+    if let targetName = call.findStringArgument(label: "name") {


Should we also check that we are indeed at a target? For example I think we shouldn’t offer this when invoking the refactoring on the call to the Package initializer or on a testTarget.

Yeah, I want to validate that we're on a kind of target we know how to build a test for.

Done in 4c1e119

DougGregor · 2024-04-23T01:03:34Z

@swift-ci please test

DougGregor · 2024-04-23T01:04:18Z

swiftlang/swift-package-manager#7477

@swift-ci please test

DougGregor · 2024-04-23T04:21:22Z

swiftlang/swift-package-manager#7477

@swift-ci please test

DougGregor · 2024-04-23T04:22:56Z

swiftlang/swift-package-manager#7477

@swift-ci please test Windows

DougGregor · 2024-04-23T05:10:32Z

swiftlang/swift-package-manager#7477

@swift-ci please test Windows

DougGregor · 2024-04-23T14:40:37Z

@swift-ci please test

DougGregor · 2024-04-23T14:40:59Z

@swift-ci please test Windows

DougGregor · 2024-04-23T15:17:05Z

@swift-ci please test Windows

ahoppen

Thanks, I have left a few more ideas inline.

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

ahoppen · 2024-04-23T20:18:57Z

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

+      let edits = try AddTarget.addTarget(target, to: scope.file)
+      return [
+        CodeAction(
+          title: "Add test target",
+          kind: .refactor,
+          edit: edits.asWorkspaceEdit(snapshot: scope.snapshot)
+        )
+      ]


Would it make sense to do some rough check whether a test target with that name already exists? I think even if we just do a textual search for "\(targetName)Tests" (ie the target name wrapped in quotes), that would cover most cases and would prevent us from showing a code action to add a test target when it obviously already exists.

If we're going to do that check, it should happen down in the SwiftPM code. Personally, I'm not sure... someone might want to use the action and then go rename the text. Are we helping them by taking the action away?

That’s a good point. Let’s leave it for now 👍🏽

ahoppen · 2024-04-23T20:19:39Z

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

+        targets: [targetName]
+      )
+
+      let edits = try AddProduct.addProduct(product, to: scope.file)


Similar here, do you think there’s a way to check if we already have a product for this target? I think textual search wouldn’t work here.

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

Tests/SourceKitLSPTests/CodeActionTests.swift

…ifier

Leverage the newly-introduced package manifest editing tools in SwiftPM to create a package editing refactoring operation. This operation can be triggered from the a target in the manifest itself, e.g., .target(name: "MyLib") and will add a test target to the package manifest that depends on this target, i.e., .testTarget( name: "MyLibTests", dependencies: [ "MyLib" ] ) It will also create a new source file `Tests/MyLibTests/MyLibTests.swift` that that imports both MyLib and XCTest, and contains an XCTestCase subclass with one test to get you started.

DougGregor · 2024-04-25T04:57:16Z

@swift-ci please test

DougGregor · 2024-04-25T04:57:27Z

@swift-ci please test Windows

DougGregor · 2024-04-25T16:31:07Z

swiftlang/swift-package-manager#7498

@swift-ci please test Windows

DougGregor · 2024-04-25T19:14:04Z

swiftlang/swift-package-manager#7498

@swift-ci please test Windows

DougGregor · 2024-04-25T21:39:55Z

swiftlang/swift#73267
swiftlang/swift-package-manager#7498

@swift-ci please test Windows

DougGregor · 2024-04-25T21:48:09Z

swiftlang/swift#73267
swiftlang/swift-package-manager#7502

@swift-ci please test Windows

DougGregor · 2024-04-25T21:50:11Z

swiftlang/swift#73267
swiftlang/swift-package-manager#7502

@swift-ci please test Windows

DougGregor · 2024-04-26T00:33:33Z

swiftlang/swift#73267

@swift-ci please test Windows

DougGregor · 2024-04-26T02:22:01Z

swiftlang/swift-package-manager#7498
swiftlang/swift#73267

@swift-ci please test Windows

karwa · 2024-04-26T08:30:19Z

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

+  }
+
+  /// A list of target kinds that allow the creation of tests.
+  static let targetsThatAllowTests: Set<String> = [


I know this isn't particularly performance-sensitive, but this should probably be an Array. A Set is overkill.

karwa · 2024-04-26T08:31:14Z

Sources/SourceKitLSP/Swift/CodeActions/PackageManifestEdits.swift

+  }
+
+  /// A list of target kinds that allow the creation of tests.
+  static let targetsThatAllowProducts: Set<String> = [


Same here. An Array would probably be better.

Yes, you're right. I'll clean it up in a subsequent PR, because I'm drowning in unmerged PRs right now

DougGregor · 2024-04-27T06:58:56Z

swiftlang/swift-package-manager#7509

@swift-ci clean test Windows

DougGregor · 2024-04-27T15:14:36Z

swiftlang/swift-syntax#2628
swiftlang/swift-package-manager#7505

@swift-ci clean test Windows

DougGregor requested review from benlangmuir and ahoppen as code owners April 20, 2024 00:44

ahoppen reviewed Apr 20, 2024

View reviewed changes

DougGregor changed the title ~~Add refactoring action for adding a test target to a package manifest~~ Add refactoring actions for adding a test targets and products to a package manifest Apr 23, 2024

DougGregor mentioned this pull request Apr 23, 2024

Add separate "add test" manifest actions for XCTest and Swift Testing #1197

Merged

DougGregor force-pushed the package-editing-refactor branch from e5d9ff0 to 950176b Compare April 23, 2024 14:40

ahoppen reviewed Apr 23, 2024

View reviewed changes

DougGregor added 6 commits April 24, 2024 21:25

Fix JSON encoding for TextDocumentEdit and VersionedTextDocumentIdent…

d11230c

…ifier

Address code review and add "Add product to export this target" action

0830a95

Reformat

5428b9c

Fixup for coding change to OptionalVersionedTextDocumentIdentifier

f6d286e

Add PackageManifestEdits.swift to CMake build

e540f16

DougGregor force-pushed the package-editing-refactor branch from 950176b to de43434 Compare April 25, 2024 04:26

DougGregor added 2 commits April 24, 2024 21:56

[CMake] Add dependency on SwiftPM's PackageModuleSyntax

502c9a8

Address code review feedback

3674e7f

DougGregor force-pushed the package-editing-refactor branch from de43434 to 3674e7f Compare April 25, 2024 04:57

karwa reviewed Apr 26, 2024

View reviewed changes

dschaefer2 mentioned this pull request Apr 27, 2024

Integration with Swift Package Index swiftlang/vscode-swift#769

Open

DougGregor merged commit e0f7221 into swiftlang:main Apr 27, 2024

DougGregor deleted the package-editing-refactor branch April 27, 2024 19:49

ahoppen mentioned this pull request May 1, 2024

Merge main into release/6.0 #1213

Merged

Add refactoring actions for adding a test targets and products to a package manifest #1193

Add refactoring actions for adding a test targets and products to a package manifest #1193

Uh oh!

Conversation

DougGregor commented Apr 20, 2024

Uh oh!

DougGregor commented Apr 20, 2024

Uh oh!

DougGregor commented Apr 20, 2024

Uh oh!

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!

DougGregor commented Apr 23, 2024

Uh oh!

DougGregor commented Apr 23, 2024

Uh oh!

DougGregor commented Apr 23, 2024

Uh oh!

DougGregor commented Apr 23, 2024

Uh oh!

DougGregor commented Apr 23, 2024

Uh oh!

DougGregor commented Apr 23, 2024

Uh oh!

DougGregor commented Apr 23, 2024

Uh oh!

DougGregor commented Apr 23, 2024

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DougGregor commented Apr 25, 2024

Uh oh!

DougGregor commented Apr 25, 2024

Uh oh!

DougGregor commented Apr 25, 2024

Uh oh!

DougGregor commented Apr 25, 2024

Uh oh!

DougGregor commented Apr 25, 2024

Uh oh!

DougGregor commented Apr 25, 2024

Uh oh!

DougGregor commented Apr 25, 2024

Uh oh!

DougGregor commented Apr 26, 2024

Uh oh!

DougGregor commented Apr 26, 2024

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