Skip to content

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Jun 22, 2025

Description

Now that all the frameworks sources are Swift packages (targets in the same Swift package to be precise) we can move the tests there, too. We didn't do that all in one go to avoid big diffs.

Notice:

  • The whole Networking/ folder has been removed, including the dedicated Xcode project
  • No changes in the test code
  • The Yosemite and WooCommerce test targets now reference the NetworkingTests/Responses/ folder from the location in the Modules/Tests/ folder. At some point, we ought to isolate the responses in some kind of tests helper package, but for the moment including them this way (which to me is like a soft-duplication) is convenient.

Testing information

See green CI.

Additionally, you can run the focused Hardware tests by selecting the Hardware scheme from Xcode.

Screenshots


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. — N.A.

@mokagio mokagio added this to the 22.8 milestone Jun 22, 2025
@mokagio mokagio self-assigned this Jun 22, 2025
@mokagio mokagio added category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. Testing labels Jun 22, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Jun 22, 2025

2 Warnings
⚠️ 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).
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jun 22, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Number30704
VersionPR #15796
Bundle IDcom.automattic.alpha.woocommerce
Commit8ca4a18
Installation URL039h07t08vapo
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@mokagio mokagio marked this pull request as ready for review June 22, 2025 23:51
@mokagio mokagio requested review from a team June 22, 2025 23:51
Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! I removed the derived data and tried running the remote tests locally, some behaviors I'd like to clarify:

  1. There's still a Networking project in the project navigator, but clicking on shows an error (first screenshot). Also seeing an error (the second screenshot) that doesn't block running the tests but I think it's worth removing to reduce confusion:
error clicking on Networking project similar error
Screenshot 2025-06-23 at 7 44 15 AM Screenshot 2025-06-23 at 7 46 31 AM
  1. In order to run Networking tests, is it an expected change to have to switch to the Networking scheme instead of staying in the WooCommerce scheme?
Screenshot 2025-06-23 at 7 44 15 AM
  1. I haven't been able to see the diamond check boxes next to each test class/function in the code to run them, do they show up on your end? Just wondering if it is an unexpected change, if so it might be worth notifying developers to prevent from unncessary time trying to get them working. I tried reopening Xcode which usually resolves it, but still cannot see them appear.
branch trunk
Screenshot 2025-06-23 at 7 51 02 AM Screenshot 2025-06-23 at 7 54 24 AM

"WordPressShared",
.product(name: "KeychainAccess", package: "KeychainAccess"),
],
resources: [.process("Responses")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The Yosemite and WooCommerce test targets now reference the NetworkingTests/Responses/ folder from the location in the Modules/Tests/ folder. At some point, we ought to isolate the responses in some kind of tests helper package, but for the moment including them this way (which to me is like a soft-duplication) is convenient.

I agree that a separate package for these mock responses would be ideal.

Base automatically changed from ainfra-778-move-storagetests-to-modules to trunk June 23, 2025 23:42
@mokagio mokagio force-pushed the ainfra-779-move-networkingtests-to-modules branch from 915a045 to 550c2eb Compare June 24, 2025 00:07
@mokagio
Copy link
Contributor Author

mokagio commented Jun 24, 2025

Thanks for the review @jaclync 🙇‍♂️

  1. There's still a Networking project in the project navigator...

Ouch! I should have double checked and removed it from the workspace. I rebased on trunk to address a merge conflict and also addressed that.

image
  1. In order to run Networking tests, is it an expected change to have to switch to the Networking scheme instead of staying in the WooCommerce scheme?

No. That's just Xcode being annoying with the test plan and rebase. See for example https://github.com/woocommerce/woocommerce-ios/pull/15794/files#r2160527954. This should be addressed now as well.

image
  1. I haven't been able to see the diamond check boxes next to each test class/function in the code to run them, do they show up on your end?

Funny that you mention that. It's a long standing Xcode bug which should be fixed in Xcode 26:

image

@mokagio mokagio enabled auto-merge June 24, 2025 00:24
@jaclync
Copy link
Contributor

jaclync commented Jun 24, 2025

Thanks for the updates! 1 is fixed now, and 3 doesn't sound like a blocking issue and should be fixed later.

2. In order to run Networking tests, is it an expected change to have to switch to the Networking scheme instead of staying in the WooCommerce scheme?

No. That's just Xcode being annoying with the test plan and rebase. See for example https://github.com/woocommerce/woocommerce-ios/pull/15794/files#r2160527954. This should be addressed now as well.

After rebasing the PR and relaunching Xcode a few times, I still wasn't able to run the NetworkingTests with the WooCommerce scheme.

Screenshot 2025-06-24 at 6 00 42 AM

More importantly, I didn't seem to find the NetworkingTests (and StorageTests which is also missing from the screenshot above) mentions in the unit tests CI logs. Not sure if I missed the tests in the CI logs?

@mokagio mokagio force-pushed the ainfra-779-move-networkingtests-to-modules branch from 550c2eb to 93ec55a Compare June 24, 2025 20:38
@mokagio
Copy link
Contributor Author

mokagio commented Jun 24, 2025

@jaclync Mmmm.... What's going on? The two targets weren't there on my branch after rebasing on trunk either. But that's very odd given the screenshot above, which I took on purpose to show they worked when I pushed.

Anyway, I committed the Test Plan update in isolation in 93ec55a.

Let's see how it goes in CI this time.

@mokagio mokagio requested review from jaclync June 25, 2025 01:57
@iamgabrielma
Copy link
Contributor

👀 Just sneaking in this PR to share that CI failed with the crash I originally saw when making StorageTests a package, so we might have some flakiness here:

🔬 Unit Tests: 1 tests have failed (1 distinct assertion failure(s) in total)
test_file_is_loaded() in FileStorageTests
Crash: xctest (2683) implicit closure #1 in FileStorageTests.test_file_is_loaded()

I haven't re-run the failing build in case you want to take a further look 👍

@mokagio
Copy link
Contributor Author

mokagio commented Jun 25, 2025

@jaclync @iamgabrielma thanks folks.

Everything should be fixed now. I clearly lost something along the way with the rebases 😓

Copy link
Contributor

@jaclync jaclync left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @mokagio! I see the NetworkingTests in the latest CI log, can run NetworkingTests without switching scheme as the NetworkingTests is included in the test plan now. :shipit:

Screenshot 2025-06-25 at 8 04 14 AM

@mokagio mokagio merged commit da2ffdc into trunk Jun 25, 2025
13 checks passed
@mokagio mokagio deleted the ainfra-779-move-networkingtests-to-modules branch June 25, 2025 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. Testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants