Skip to content

Integrate WordPressKit and WordPressAuthenticator unit tests#23367

Merged
crazytonyli merged 13 commits intotrunkfrom
tonyli-move-wpkit-tests
Jun 20, 2024
Merged

Integrate WordPressKit and WordPressAuthenticator unit tests#23367
crazytonyli merged 13 commits intotrunkfrom
tonyli-move-wpkit-tests

Conversation

@crazytonyli
Copy link
Contributor

@crazytonyli crazytonyli commented Jun 18, 2024

Note

This PR is built on top of #23366

This PR integrates the exiting unit tests of WordPressKit and WordPressAuthenticator into Xcode and CI.

From Xcode, you run their unit test from the Test navigator.

Screenshot 2024-06-18 at 1 50 03 PM

@crazytonyli crazytonyli force-pushed the tonyli-move-wpkit-tests branch 2 times, most recently from fa87ec0 to 6af8dc5 Compare June 18, 2024 01:36
@crazytonyli crazytonyli force-pushed the tonyli-move-wpkit-tests branch from 6af8dc5 to 4bd1c0c Compare June 18, 2024 01:43
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 18, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23367-a6e4252
Version25.1
Bundle IDorg.wordpress.alpha
Commita6e4252
App Center BuildWPiOS - One-Offs #10208
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jun 18, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23367-a6e4252
Version25.1
Bundle IDcom.jetpack.alpha
Commita6e4252
App Center Buildjetpack-installable-builds #9257
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@crazytonyli crazytonyli requested a review from jkmassel June 18, 2024 01:54
@crazytonyli crazytonyli added the Tooling Build, Release, and Validation Tools label Jun 18, 2024
@crazytonyli crazytonyli added this to the 25.2 milestone Jun 18, 2024
@crazytonyli crazytonyli marked this pull request as ready for review June 18, 2024 01:54
@crazytonyli crazytonyli requested a review from kean June 20, 2024 02:01
@kean
Copy link
Contributor

kean commented Jun 20, 2024

  • Is this change in the right order? The next PR seems to be undo-ing a large part of it because instead of CocoaPods it uses Xcode targets for these two new modules.
  • (enhancement) This change adds a large amount of tests to the project and two separate jobs that will have to run for every PR. The WPKit and WPAuthenticator tests themselves run relatively fast, but there is a non-trivial amount of time spent in Setting up Gems and Setting up Pods. I would suggest adding unit tests to the existing wordpress-unit-tests that already runs tests for multiple targets.
  • (nit) I would name it WordPressKitTests instead of WordPressKit-Unit-Tests to be consistent with other test targets
  • Just noting that WPKit has some flaky tests, adding to the existing flaky test suite. The more flaky tests, the more the chance one of them fails the build. I would suggest removing the flaky ones – no big loss.

@crazytonyli
Copy link
Contributor Author

@kean The original plan is just moving WPKit and WPAuthenticator to this repo, which is completed once this PR is merged. But later, I thought I'll go ahead to try replacing CocoaPods pods with Swift Packages, since WPKit was the blocker from adopting Swift Package in our own libraries. That went pretty well, so I carried on and replaces as many CocoaPods pods as I can, which is done in #23378.

This change adds a large amount of tests to the project and two separate jobs that will have to run for every PR.

This CI step is gone in #23378. They now run alongside with WordPress unit tests, as you suggested.

I would name it WordPressKitTests instead of WordPressKit-Unit-Tests

This is also done in #23378. BTW, the name "Xxx-Unit-Tests" was created by CocoaPods. We don't have much control over it.

Just noting that WPKit has some flaky tests

I'll try rerun WPKit unit tests locally (and possibly on CI too), to see if I can fix those flaky tests. Removing them can be the last resort if we can't fix them properly.

Base automatically changed from tonyli-move-wpkit to trunk June 20, 2024 21:49
@crazytonyli
Copy link
Contributor Author

@kean Can we get this PR reviewed and merged first? I'll follow up with a separate PR if unit tests require some fixes.

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 25.2. This milestone is due in less than 4 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@kean
Copy link
Contributor

kean commented Jun 20, 2024

@kean Can we get this PR reviewed and merged first? I'll follow up with a separate PR if unit tests require some fixes.

Sure, I'm not suggesting to redo anything – was just clarifying.

This CI step is gone in #23378. They now run alongside with WordPress unit tests

Nice!

Copy link
Contributor

@kean kean left a comment

Choose a reason for hiding this comment

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

LGTM
*as long as the tests completes and assuming that most of it is reworked in the next PR. The source changes to the tests look good.

@crazytonyli
Copy link
Contributor Author

[2024-06-20T22:12:50Z] Errors:
[2024-06-20T22:12:50Z] - [ ] WordPressAuthenticator/WordPressAuthenticator.podspec#L1: Naming/FileName: The name of this source file (`WordPressAuthenticator.podspec`) should use snake_case.
[2024-06-20T22:12:50Z] - [ ] WordPressAuthenticator/WordPressAuthenticator.podspec#L3: Metrics/BlockLength: Block has too many lines. [38/25]
[2024-06-20T22:12:50Z] - [ ] WordPressKit/WordPressKit.podspec#L1: Naming/FileName: The name of this source file (`WordPressKit.podspec`) should use snake_case.
[2024-06-20T22:12:50Z] - [ ] WordPressKit/WordPressKit.podspec#L3: Metrics/BlockLength: Block has too many lines. [35/25]

I'm going to ignore the above issues reported by danger, because these podspec files will be deleted soon.

@crazytonyli crazytonyli merged commit 6dc78ec into trunk Jun 20, 2024
@crazytonyli crazytonyli deleted the tonyli-move-wpkit-tests branch June 20, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Tooling Build, Release, and Validation Tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants