[UI Tests] Run disableAutoFillPasswords once before all the UI Tests. #21148

tiagomar · 2023-07-20T04:51:49Z

Description

The objective of this PR is to make disableAutoFillPasswords() run only once before all the whole suite as the change is persisted in the simulators and doesn't need to run before each test. It was done by adding a TestObserver and running disableAutoFillPasswords()
within testBundleWillStart(_:). h/t to @mokagio for the tip!
I also added some waiting for the elements because, even though it will run only once per simulator, it's still navigating through UI and still can be impacted by eventual CI slowness.

Testing

Ci should be 🟢

tiagomar · 2023-07-20T05:05:25Z

WordPress/UITestsFoundation/TestObserver.swift

+    func executeWithRetries(_ operation: () -> Bool) {
+        var retryCount = 3
+
+        while !operation() && retryCount > 0 {
+            retryCount -= 1
+        }
+    }


Retrying to be on the safer side as all the tests would fail in case this is not disabled. I have seen it happening a couple times in a week.
I tried to make it reusable as we might need to change other setting in the future.

tiagomar · 2023-07-20T05:14:36Z

WordPress/UITestsFoundation/TestObserver.swift

+        guard passwordsMenuItem.waitForIsHittable() else {
+            XCTFail("SetUp Failed: Passwords menu item was not hittable in Settings.")
+            return false
+        }
+        settings.staticTexts["Passwords"].tap()


There are a few guards like this which I'd like to DRY it but I couldn't use waitAndTap( _ element: XCUIElement, maxRetries: Int = 10, timeout: TimeInterval = 10) from Globals.swift. It worked when I added JetpackUITests Target Membership to Globals.swift, but It broke a dozen of other things. 😅

or maybe we can add it as a private func in the same class to call in this method, it won't be reusable elsewhere but at least it's reusable here. wdyt?

Yeah, I started doing it and then I noticed I was almost duplicating waitForIsHittable(), struggling with the Boolean returns to control the retry flow, and all of that because of the XCTFail(String) messages that would only help debugging locally. Since the local logs are enough to debug a possible issue, I decided to make it simple and rely on waitForIsHittable().
Changed in 798de02.

wpmobilebot · 2023-07-20T05:16:21Z

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

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr21148-fb8d77d
	Version	22.8
	Bundle ID	com.jetpack.alpha
	Commit	`fb8d77d`
	App Center Build	jetpack-installable-builds #5505

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

wpmobilebot · 2023-07-20T05:17:11Z

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

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr21148-fb8d77d
	Version	22.8
	Bundle ID	org.wordpress.alpha
	Commit	`fb8d77d`
	App Center Build	WPiOS - One-Offs #6475

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

tiagomar · 2023-07-20T05:19:05Z

WordPress/UITestsFoundation/TestObserver.swift

+
+        let passwordOptions = settings.staticTexts["Password Options"]
+        guard passwordOptions.waitForIsHittable() else {
+            XCTFail("SetUp Failed: Password Options was not hittable in Passwords.")


I added these XCTFail(String) mostly for local debugging as it doesn't actually make the tests fail because it runs before the test bundle even start.

jostnes

👋 @tiagomar, thanks for improving the implementation! just looking at the bonus time savings is already satisfying 💯

i added some comments, nothing blocking. and also an additional question about removeTestObserver(_:) (also TIL about TestObserver) since it is added at the start of the test run, do we need to remove it at the end of the test run?

jostnes · 2023-07-20T06:30:26Z

WordPress/WordPress.xcodeproj/project.pbxproj

+		EA85B7AB2A686C7A0096E097 /* TestObserver.swift in Sources */ = {isa = PBXBuildFile; fileRef = EA85B7A92A6860370096E097 /* TestObserver.swift */; };
+		EA85B7AF2A688AB00096E097 /* TestObserver.swift in Sources */ = {isa = PBXBuildFile; fileRef = EA85B7A92A6860370096E097 /* TestObserver.swift */; };
+		EA85B7B02A688B1C0096E097 /* TestObserver.swift in Sources */ = {isa = PBXBuildFile; fileRef = EA85B7A92A6860370096E097 /* TestObserver.swift */; };


are these expected, or are leftovers from adding/removing the file multiple times?

(if it's the latter, which always happens to me, i usually discard all changes in the ./project.pbxproj before submitting the changes and rebuild the project so the references will be added to the right place in the file)

Good catch!! 👏
I did remove one of them in 937cae8, the other two are related to JetpackUI Tests and WordPressUITests build phase references.

jostnes · 2023-07-20T06:35:02Z

WordPress/UITestsFoundation/TestObserver.swift

+
+    func disableAutoFillPasswords() -> Bool {
+        let settings = XCUIApplication(bundleIdentifier: "com.apple.Preferences")
+        settings.terminate()


Do we need this .terminate() here? If this will be run once at the start of the test run, is this necessary? I'm thinking there's nothing to terminate at that point but I could be missing something.

This is needed in a retry case if something goes wrong in the first execution and also to make our lives easier when running multiple times locally. =]

I added a comment in 798de02.

jostnes · 2023-07-20T06:38:12Z

WordPress/UITestsFoundation/TestObserver.swift

+        guard passwordsMenuItem.waitForIsHittable() else {
+            XCTFail("SetUp Failed: Passwords menu item was not hittable in Settings.")
+            return false
+        }
+        settings.staticTexts["Passwords"].tap()


or maybe we can add it as a private func in the same class to call in this method, it won't be reusable elsewhere but at least it's reusable here. wdyt?

jostnes

Thanks for addressing the comments, the changes look good!

Run disableAutoFillPasswords once before all the UI Tests.

6da5d28

tiagomar added the Testing Unit and UI Tests and Tooling label Jul 20, 2023

tiagomar added this to the Someday milestone Jul 20, 2023

tiagomar requested a review from mokagio July 20, 2023 04:51

tiagomar mentioned this pull request Jul 20, 2023

[UI Tests] Disable Auto Fill Passwords for UI Tests #21034

Merged

tiagomar commented Jul 20, 2023

View reviewed changes

jostnes reviewed Jul 20, 2023

View reviewed changes

tiagomar added 2 commits July 20, 2023 12:05

Remove stale references from .xcodeproj

937cae8

Remove test observer and reduce code duplication.

798de02

tiagomar marked this pull request as ready for review July 20, 2023 18:10

tiagomar requested a review from a team as a code owner July 20, 2023 18:10

tiagomar requested a review from jostnes July 20, 2023 21:16

jostnes approved these changes Jul 21, 2023

View reviewed changes

tiagomar added 2 commits July 21, 2023 11:10

Guard waitForIsHittables to allow the retry flow in case of failure.

af49f74

Fix a typo

fb8d77d

tiagomar merged commit e531eac into trunk Jul 21, 2023

tiagomar deleted the ui-tests-before-all branch July 21, 2023 20:36

[UI Tests] Run disableAutoFillPasswords once before all the UI Tests. #21148

[UI Tests] Run disableAutoFillPasswords once before all the UI Tests. #21148

Uh oh!

Conversation

tiagomar commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

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!

wpmobilebot commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Jul 20, 2023 • 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!

jostnes 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!

jostnes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tiagomar commented Jul 20, 2023 •

edited

Loading

wpmobilebot commented Jul 20, 2023 •

edited

Loading

wpmobilebot commented Jul 20, 2023 •

edited

Loading