Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test Fixes to Run Xcode 15 in CI #22270

Merged
merged 16 commits into from
Jan 8, 2024
Merged

Test Fixes to Run Xcode 15 in CI #22270

merged 16 commits into from
Jan 8, 2024

Conversation

jostnes
Copy link
Contributor

@jostnes jostnes commented Dec 22, 2023

Description

This is the "cleaned up" version of @mokagio's PR #21921, with only the final working commit and the many experimentation commits from the original PR omitted (see original PR for more description about the changes)

The branch was tested on WPiOS-macv2-test with the UI test steps passing on every build. There are still some flaky tests, but automated retries currently handle those, and it would be better to handle them separately if they become too flaky. The current run time for UI test is still within 30 minutes (same as before).

The UI test fixes that were made:

  1. Somehow stats test started failing/became flakier with this change. I updated the stats test and mocks to be more dynamic; the year is no longer hard coded to 2019 but follows the current year. Flakiness is still seen when the screen is not updated to use the values presented in the mocks; I couldn't reproduce this on a real device, which is why this could be a mock-related issue.
  2. On reply notification, another flakiness is seen where the tap does not work (based on screenshots, the tap did happen, but the action that should have happened after the tap did not. Also, it is not reproducible on a real device). I renamed tapUntilCondition to waitForElementAndTap and updated it to cover more conditions to make it more robust. The change works for now.
  3. To workaround the simulator bug where we can't turn off Save Password from settings, app.dismissSavePasswordPrompt() was readded after password screen.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist: Not a UI PR

@jostnes jostnes requested a review from a team as a code owner December 22, 2023 05:57
@tiagomar
Copy link
Contributor

tiagomar commented Jan 2, 2024

The executeWithRetries(disableAutoFillPasswords) in testBundleWillStart from TestObserver is still running. Is the idea still to skip it for now)?

Copy link
Contributor

@tiagomar tiagomar 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 working on this, @jostnes ! 🙇

I've run all tests locally on Xcode 15.1 and all tests passed on iPhone and iPad. I just left a couple comments before approving this PR.

@@ -62,20 +62,29 @@ public func waitAndTap( _ element: XCUIElement, maxRetries: Int = 20) {
}
}

public func tapUntilCondition(element: XCUIElement, condition: Bool, description: String, maxRetries: Int = 10) {
public func waitForElementAndTap(_ tapElement: XCUIElement, untilConditionOn conditionElement: XCUIElement, condition: String, errorMessage: String, maxRetries: Int = 10) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure waitForElementAndTap describes exactly what the function does? The function doesn't actually wait for the tapElement before tapping on it.

WDYT about tapUntil(_ tapElement: XCUIElement, conditionOn conditionElement: XCUIElement, condition: String, errorMessage: String, maxRetries: Int = 10)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tiagomar 's observation resonates with me. As a matter of fact, I opened #22322 to address it (with an attempt to improve upon it).

Having said that, I don't see this as a blocker to merge given the benefit that merging this PR to add Xcode 15.1 / iOS 17.2 support would bring to the dev teams.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, @mokagio, it makes sense to move on with this PR and address the naming in another one. 👍

@mokagio mokagio added this to the 24.0 milestone Jan 3, 2024
@mokagio mokagio added the Tooling Build, Release, and Validation Tools label Jan 3, 2024
"Typed" in that it expects an `enum` instead of a `String` to define the
expected element state.
It became unused in the previous commit,
b442842
Copy link
Contributor

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

@jostnes @tiagomar I'd love to see this merged sooner rather than later, even if it means not addressing the naming comment straightaway and instead follow up later. Rationale: the sooner we officially support Xcode 15.1 and iOS 17+ in CI, the better.

@@ -62,20 +62,29 @@ public func waitAndTap( _ element: XCUIElement, maxRetries: Int = 20) {
}
}

public func tapUntilCondition(element: XCUIElement, condition: Bool, description: String, maxRetries: Int = 10) {
public func waitForElementAndTap(_ tapElement: XCUIElement, untilConditionOn conditionElement: XCUIElement, condition: String, errorMessage: String, maxRetries: Int = 10) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@tiagomar 's observation resonates with me. As a matter of fact, I opened #22322 to address it (with an attempt to improve upon it).

Having said that, I don't see this as a blocker to merge given the benefit that merging this PR to add Xcode 15.1 / iOS 17.2 support would bring to the dev teams.

@staskus
Copy link
Contributor

staskus commented Jan 3, 2024

Note: I have widget API compliance for Xcode 15 + iOS 17 lined up (#21705). So it's not a blocker if you notice anything wrong related to widgets.

@tiagomar tiagomar self-requested a review January 3, 2024 12:30
Copy link
Contributor

@tiagomar tiagomar left a comment

Choose a reason for hiding this comment

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

Approving this PR now as the naming issue will be addressed in #22322.

@jostnes
Copy link
Contributor Author

jostnes commented Jan 4, 2024

There's a unit test failing in the PR. I'll update with the latest trunk again to see if it solves the issue and to be able to merge this.

@wordpress-mobile wordpress-mobile deleted a comment from jacob121532 Jan 4, 2024
@tiagomar
Copy link
Contributor

tiagomar commented Jan 4, 2024

There's a unit test failing in the PR. I'll update with the latest trunk again to see if it solves the issue and to be able to merge this.

Yeah, it has been taken care of in 7365c10. #22322 is ready to be merged into this branch.

@jostnes
Copy link
Contributor Author

jostnes commented Jan 4, 2024

@mokagio the build is good on the iOS 17+ pipeline in CI but some steps are failing on the pipeline checks linked to this PR because of Unable to find remote image: xcode-15.1 (which i guess is expected)

should the GitHub checks be updated for this? at least one of the failing step is marked as required so we can't merge this yet.

@mokagio
Copy link
Contributor

mokagio commented Jan 4, 2024

@jostnes yes sorry, I should have left a note that before being able to merge this we need to move WordPress-iOS on Buildkite to the Apple Silicon cluster.

I have a PR for it that I'll mention internally.

@guarani
Copy link
Contributor

guarani commented Jan 4, 2024

Note: I have widget API compliance for Xcode 15 + iOS 17 lined up (#21705). So it's not a blocker if you notice anything wrong related to widgets.

👋 Will the 24.0 beta be built using Xcode 15? If so, I think #21705 needs to be merged before Monday to avoid widgets breaking in the beta builds. Apologies, it's been waiting for my review for a while now, I'll get to it ASAP.

@mokagio mokagio enabled auto-merge January 8, 2024 02:19
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 8, 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 Numberpr22270-5bafb56
Version23.9
Bundle IDorg.wordpress.alpha
Commit5bafb56
App Center BuildWPiOS - One-Offs #8342
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 Jan 8, 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 Numberpr22270-5bafb56
Version23.9
Bundle IDcom.jetpack.alpha
Commit5bafb56
App Center Buildjetpack-installable-builds #7365
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@peril-wordpress-mobile
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@wpmobilebot
Copy link
Contributor

1 Warning
⚠️ This PR is assigned to the milestone 24.0. 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

@mokagio mokagio merged commit 7b65cbc into trunk Jan 8, 2024
23 checks passed
@mokagio mokagio deleted the ui-test-fixes-ios17.2 branch January 8, 2024 04:33
mokagio added a commit that referenced this pull request Jan 8, 2024
…ring

This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
…-widgets-xcode-15-ios-17

This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270

---

There were conflicts because the Gutenberg version move forward on
`trunk` and this branch. I resolved by keeping Gutenberg to the version
here because that's the point of this branch, but given the time delta
it's possible the build will fail.
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
mokagio added a commit that referenced this pull request Jan 8, 2024
This is necessary to get up to date with a CI configuration change that
requires running on Xcode 15.1.

See #22270
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.

None yet

7 participants