Reset Modules as an Xcode local package with tests in CI #24518

mokagio · 2025-05-05T06:04:44Z

Description

#24511 successfully addressed the issue of duplicated packages in the target selection.

Unfortunately, it resulted in all local packages (those from Modules/ that is) no longer being accessible to the unit tests runner—see internal discussion in p1746169066574499/1745343713.430699-slack-C08HQR4C2TS

This PR propose a different fix, achieved by re-adding Modules/ as a local package not a package dependency, following the instructions from https://developer.apple.com/documentation/xcode/organizing-your-code-with-local-packages.

This setup maintains the single result in the target search...

...while also allowing for Modules/ to run as dedicated schemes or as part of a test plan

Also notice, we have so many tests (and warnings when running them!) that the logs don't show the first tests that run, but if you check the XML report, you'll see that indeed we are running all previously disabled tests

Next steps

Rename WordPressUnitTests.xctestplan to Keystone.xctestplan—to do in dedicated PR to avoid diff noise here
Ensure all module targets that have tests run as part as Keystone.xctestplan
Add dedicated schemes for focused package development

https://linear.app/a8c/issue/AINFRA-428/investigate-wjr-setup-that-addresses-duplicated-modules-while

dangermattic · 2025-05-05T06:05:18Z

	1 Warning
⚠️	`Modules/Package.swift` was changed without updating its corresponding `Package.resolved`. Please resolve the Swift packages as appropriate to your project setup (e.g. in Xcode or by running `swift package resolve`).

Generated by 🚫 Danger

wpmobilebot · 2025-05-05T06:15:43Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Configuration	Release-Alpha
	Build Number	`27638`
	Version	`PR #24518`
	Bundle ID	`com.jetpack.alpha`
	Commit	`35e360b`
	Installation URL	4ou60b6lvr0io

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

wpmobilebot · 2025-05-05T06:15:44Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Configuration	Release-Alpha
	Build Number	`27638`
	Version	`PR #24518`
	Bundle ID	`org.wordpress.alpha`
	Commit	`35e360b`
	Installation URL	7q8s5eep52dho

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

mokagio · 2025-05-05T08:20:31Z

Modules/Package.swift

-        .testTarget(name: "WordPressFluxTests", dependencies: ["WordPressFlux"], swiftSettings: [.swiftLanguageMode(.v5)]),
+        .testTarget(
+            name: "WordPressFluxTests",
+            dependencies: ["WordPressFlux"],
+            exclude: ["WordPressFluxTests.xctestplan"],
+            swiftSettings: [.swiftLanguageMode(.v5)]
+        ),


I added this to see how WordPressFluxTests would run tests with standalone scheme.

Interestingly, I tried using a copy-paste friendly exclude: ["*.xctestplan"], pattern, but it failed to find the file.

My autogenerated scheme picks up the test plan:

However, I got an unlock prompt when I edited it (but no xcsheme change in the project, possibly because the scheme might be in Modules/.swiftpm) so I'd be curious to know if you see it too...

mokagio · 2025-05-05T08:22:33Z

WordPress/WordPress.xcodeproj/project.pbxproj

-			relativePath = ../Modules;
-		};
-/* End XCLocalSwiftPackageReference section */
-


I think the difference between this and the previous setup (pre #24511) is that we no longer have any local swift package reference as a dependency for WordPress.

My guess is that, whichever way it ended up like that, the duplicated reference—as a local module in the workspace and as a package reference—is what resulted in the duplicated entries shown in the #24511 description.

kean

This looks good. Let's switch back to the standard "drag-n-drop" approach. I left a couple of small comments.

kean · 2025-05-07T13:42:53Z

Modules/Package.swift

@@ -6,6 +6,7 @@ let package = Package(
    name: "Modules",
    platforms: [
        .iOS(.v16),
+        .macOS(.v14), // Here just to avoid errors when building directly with `swift build`


Is this addition necessary? Most of our packages do not support macOS and won't compile with it as a target.

I find it handy to have just so swift build throws legitimate errors with the code rather than with the versions.

In particular, it was necessary to run the WordPressFlux tests via swift test. However, as I try to reproduce, I can't get past some UIKit errors, which I find odd when focused on WordPressFlux, but don't have bandwidth to investigate.

Removed it.

Thanks! Yeah, I'd remove it since it doesn't support macOS.

kean · 2025-05-07T13:43:51Z

WordPress.xcworkspace/contents.xcworkspacedata

@@ -1,6 +1,9 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <Workspace
   version = "1.0">
+   <FileRef


Let's add Modules to a project and not a workspace. We we going to remove the workspace.

We we going to remove the workspace.

I started looking at it #24534

kean · 2025-05-08T14:15:03Z

.gitignore

-#
-# Ignore the Package.resolved file. The packages source of truth is the
-# resolved file generated by the Xcode workspace.
-Modules/Package.resolved


I thought you had to use ...ace/xcshareddata/swiftpm/Package.resolved managed by Xcode? Modules/Package.resolved isn't, or is it? How did you get it to work? When you open the Xcode project, does it use pins from Modules/Package.resolved?

...ace/xcshareddata/swiftpm/Package.resolved → Modules/Package.resolved

Apologies @kean this change surprised me a bit and I should have preempted your question writing about it.

Before

Modules was part of the workspace. When Xcode detected changes in Modules/Package.swift, it triggered the packages resolution and updated the resolved file in the workspace xcshareddata folder.

After

Modules is part of the project, but is not a dependency of the project:

When Xcode detects a change in Modules/Package.swift, it triggers the packages resolution and updates Modules/Package.resolved.

I guess that if we were to add additional package dependencies to the project Xcode would then re-introduce the workspace resolved file.

When Xcode detects a change in Modules/Package.swift, it triggers the packages resolution and updates Modules/Package.resolved.

I verified this by switching color-studio from pointing to a branch to a commit. The screenshot below shows Xcode highlighting the change in the gutter and the package resolution running.

35e360b tracks the changes and the .resolved update.

Given OCLint is something we should drop soon, because it's only for Objective-C and we want to reduce that code surface.

Locking dependencies to commits makes updating them a bit more burdensome, but at least it makes the update intentional and not automatic and surprising. This change was done to test Xcode's behavior in monitoring `Modules/Package.swift` and updating `Modules/Package.resolved`. See #24518 (comment)

sonarqubecloud · 2025-05-09T01:39:07Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mokagio changed the title ~~Reset Modules as an Xcode local package~~ Reset Modules as an Xcode local package with tests in CI May 5, 2025

mokagio marked this pull request as ready for review May 5, 2025 08:07

mokagio added this to the 26.1 milestone May 5, 2025

mokagio added the Testing Unit and UI Tests and Tooling label May 5, 2025

mokagio requested review from kean and crazytonyli May 5, 2025 08:08

mokagio self-assigned this May 5, 2025

mokagio commented May 5, 2025

View reviewed changes

kean reviewed May 7, 2025

View reviewed changes

mokagio force-pushed the mokagio/restore-spm-local-modules branch 2 times, most recently from 76d9467 to 8e08ab4 Compare May 8, 2025 06:27

mokagio enabled auto-merge May 8, 2025 06:34

mokagio mentioned this pull request May 8, 2025

Address a few warnings in WordPressData #24528

Merged

kean self-requested a review May 8, 2025 13:56

kean reviewed May 8, 2025

View reviewed changes

mokagio added 9 commits May 9, 2025 11:20

Add a dummy local package—We'll try to put the Modules there next

40dc57d

Replace ModulesAlterantive with Modules

758f862

Re-enable all the test targets in WordPressUnitTests

7e2e890

Delete unnecessary OCLint scheme

e2465cc

Given OCLint is something we should drop soon, because it's only for Objective-C and we want to reduce that code surface.

Configure WordPressFlux scheme to run tests

b17efe1

Move Modules package reference from workspace to project

f6dd3c4

Use the shared setup CI script in the complete code freeze step

38c0d8a

Add workaround for package path support missing

cc744d8

mokagio force-pushed the mokagio/restore-spm-local-modules branch from 8e08ab4 to 35e360b Compare May 9, 2025 01:36

mokagio requested a review from kean May 11, 2025 21:35

kean approved these changes May 12, 2025

View reviewed changes

mokagio added this pull request to the merge queue May 12, 2025

Merged via the queue into trunk with commit 5e37384 May 12, 2025
32 of 34 checks passed

mokagio deleted the mokagio/restore-spm-local-modules branch May 12, 2025 19:46

Reset Modules as an Xcode local package with tests in CI #24518

Reset Modules as an Xcode local package with tests in CI #24518

Uh oh!

Conversation

mokagio commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Next steps

Uh oh!

dangermattic commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kean 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

Before

After

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 9, 2025

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

mokagio commented May 5, 2025 •

edited

Loading

dangermattic commented May 5, 2025 •

edited

Loading

wpmobilebot commented May 5, 2025 •

edited

Loading

wpmobilebot commented May 5, 2025 •

edited

Loading