[UI Tests] Minor UI Tests code refactoring. #18349

pachlava · 2023-05-02T07:38:04Z

Description

While dealing with recent increase in UI tests fails on FTL, I firstly started looking into the tests code to search for possible improvements. The flakiness was caused by FTL service disruption, however, the changes I made to the code still make sense, IMO.

(This is also an explanation for the misleading branch name).

Each change was made in a separate commit, so it can be reverted, if you find this necessary. I recommend reviewing this PR by looking into commits separately.

Testing

CI is green

wpmobilebot · 2023-05-02T07:56:08Z

📲 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
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr18349-85ffcbf
	Commit	`85ffcbf`
	Direct Download	`wordpress-prototype-build-pr18349-85ffcbf.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2023-05-02T07:56:09Z

📲 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
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr18349-85ffcbf
	Commit	`85ffcbf`
	Direct Download	`jetpack-prototype-build-pr18349-85ffcbf.apk`

Note: Google Login is not supported on these builds.

pachlava · 2023-05-04T07:01:34Z

WordPress/src/androidTest/java/org/wordpress/android/e2e/StatsTests.kt

+        // We're not running Stats tests for JP.
+        // See https://github.com/wordpress-mobile/WordPress-Android/issues/18065
+        assumeTrue(!BuildConfig.IS_JETPACK_APP)
+


When it was in the original place, the test was skipped for JP, as it should be, but setUp was still executed, which took ~20 seconds. Since there's only one test in this class, keeping it here does not affect any other tests.

pachlava · 2023-05-04T07:10:21Z

WordPress/src/androidTest/java/org/wordpress/android/support/BaseTest.java

-        if (isElementDisplayed(R.id.continue_with_wpcom_button) || isElementDisplayed(R.id.login_open_email_client)) {
-            return;
-        }
-


The removed code didn't add much value, IMO, removing it changes nothing in the logic, and it also means two checks less. The method will still logout only if it sees the navbar.

can't help but wonder why this was added in the first place, the only reason i could think of is some potential speed improvements (not sure how much though, if any). but looking at it, i would agree with you that it doesn't seem to add much value

I assume the logic behind this was "firstly, escape the function if we are already in the logged out state". But it turned out that it's faster to check that we are in the logged in state.

pachlava · 2023-05-04T07:11:51Z

WordPress/src/androidTest/java/org/wordpress/android/e2e/flows/LoginFlow.kt

-            val negativeButton =
-                Espresso.onView(ViewMatchers.withId(R.id.quick_start_prompt_dialog_button_negative))
-            WPSupportUtils.waitForElementToBeDisplayed(negativeButton)
-            WPSupportUtils.clickOn(negativeButton)


clickOn already contains waitForElementToBeDisplayed, so it was redundant here. Also, I went for passing the identifer to the function, instead of the ViewMatcher, to keep it shorter.

pachlava · 2023-05-04T07:12:38Z

WordPress/src/androidTest/java/org/wordpress/android/e2e/BlockEditorTests.kt

@@ -32,7 +32,6 @@ class BlockEditorTests : BaseTest() {
    fun e2ePublishSimplePost() {
        val title = "publishSimplePost"
        MySitesPage()
-            .go()


After login, the app is already in needed section, no need to go there again.

pachlava · 2023-05-04T07:13:49Z

WordPress/src/androidTest/java/org/wordpress/android/e2e/LoginTests.kt

-
-    @After
-    fun tearDown() {
-        logoutIfNecessary()
-    }


Every setUp method (for all tests) contains logoutIfNecessary, so I saw no value in also having a teardown that does the same.

usually the "clean up" happens after the test ends, curious to know your reason for keeping the one in setUp() instead of tearDown()?

I'd advocate that every test is responsible for setting up the right environment before it starts, and not when it ends. Thus from my experience, it's always more resilient to handle prerequisites in setUp.

Only LoginTests.kt and SignUpTest.kt contained both setUp and teardDown with logoutIfNecessary, while the remaining five files contain only setUp with logoutIfNecessary. I removed the ones from tearDown because of a smaller footprint.

i've personally used both setUp() and tearDown() to "clean up" test states, depending on what it does and what the end states of the tests are. in the case where we are deleting the app (what JPiOS is doing), it would make sense to do it during tearDown() and from your reasoning for this one, this change makes sense to me since it was already in setUp() in the other files 👍

pachlava · 2023-05-04T07:15:35Z

WordPress/src/androidTest/java/org/wordpress/android/e2e/DashboardTests.kt

+        // We run the class for JP only (so far the class contains
+        // only a test for Domains card, which in not valid for WP)
+        assumeTrue(BuildConfig.IS_JETPACK_APP)
+


All changes in this file is a cherry-pick from #18349

did you mean to link to a different PR/commit? currently that link is this PR

Yes, sorry, I meant #18365

I meant #18365

Are you sure? That PR wasn't yet merged.

I'm sorry, @ovitrif. I hope third time's a charm 😅: #18345

jostnes

thanks for making these updates and removing those unnecessary lines @pachlava! since this involves multiple tests, i'm depending on the CI results for this.

the only outstanding question i have is on the position of logoutIfNecessary(), the other changes look good to me!

jostnes · 2023-05-08T04:34:57Z

WordPress/src/androidTest/java/org/wordpress/android/e2e/DashboardTests.kt

+        // We run the class for JP only (so far the class contains
+        // only a test for Domains card, which in not valid for WP)
+        assumeTrue(BuildConfig.IS_JETPACK_APP)
+


did you mean to link to a different PR/commit? currently that link is this PR

jostnes · 2023-05-08T04:37:30Z

WordPress/src/androidTest/java/org/wordpress/android/e2e/LoginTests.kt

-
-    @After
-    fun tearDown() {
-        logoutIfNecessary()
-    }


usually the "clean up" happens after the test ends, curious to know your reason for keeping the one in setUp() instead of tearDown()?

jostnes · 2023-05-08T04:54:20Z

WordPress/src/androidTest/java/org/wordpress/android/support/BaseTest.java

-        if (isElementDisplayed(R.id.continue_with_wpcom_button) || isElementDisplayed(R.id.login_open_email_client)) {
-            return;
-        }
-


can't help but wonder why this was added in the first place, the only reason i could think of is some potential speed improvements (not sure how much though, if any). but looking at it, i would agree with you that it doesn't seem to add much value

pachlava · 2023-05-08T09:40:19Z

the only outstanding question i have is on the position of logoutIfNecessary(), the other changes look good to me!

Thank you for the review, @jostnes 🙇 I answered the question, hope it makes sense. Could you please take another look? Thanks!

jostnes

CI is ✅

Truly skip Stats test for JP

730c87b

pachlava requested a review from a team as a code owner May 2, 2023 07:38

pachlava changed the base branch from trunk to add-domains-card-ui-test May 2, 2023 07:39

Disabled Domains Card test for WP

414cc9d

pachlava marked this pull request as draft May 2, 2023 08:52

pachlava added 4 commits May 2, 2023 14:35

Removed redundant code from logoutIfNecessary

36aa9fa

Removed redundant wait for element

b939295

Removed redundant go() from Block Editor Tests

fd1d70b

Removed excessive teardowns

85ffcbf

pachlava commented May 4, 2023

View reviewed changes

pachlava changed the title ~~[UI Tests] Examine flaky tests~~ [UI Tests] Minor UI Tests code refactoring. May 4, 2023

pachlava commented May 4, 2023

View reviewed changes

pachlava added the UI Tests Anything related to automated UI Tests. label May 4, 2023

pachlava added this to the Future milestone May 4, 2023

pachlava marked this pull request as ready for review May 4, 2023 07:17

Base automatically changed from add-domains-card-ui-test to trunk May 4, 2023 11:31

jostnes reviewed May 8, 2023

View reviewed changes

pachlava requested a review from jostnes May 8, 2023 09:40

jostnes approved these changes May 8, 2023

View reviewed changes

ParaskP7 mentioned this pull request May 8, 2023

[Dependency Updates] Update androidxComposeBomVersion to 2023.05.00 #18378

Merged

17 tasks

pachlava merged commit 0c64cb8 into trunk May 8, 2023
18 checks passed

pachlava deleted the examine-flaky-tests branch May 8, 2023 10:48

ParaskP7 mentioned this pull request May 8, 2023

[Dependency Updates] Main Batch - AndroidX Compose/Kotlin (plus Core) #18364

Merged

18 tasks

pachlava mentioned this pull request May 9, 2023

[UI Tests] Restored the teardowns in Login and Signup tests. #18387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UI Tests] Minor UI Tests code refactoring. #18349

[UI Tests] Minor UI Tests code refactoring. #18349

pachlava commented May 2, 2023 •

edited

wpmobilebot commented May 2, 2023 •

edited

wpmobilebot commented May 2, 2023 •

edited

pachlava May 4, 2023

pachlava May 4, 2023

jostnes May 8, 2023

pachlava May 8, 2023

pachlava May 4, 2023

pachlava May 4, 2023

pachlava May 4, 2023

jostnes May 8, 2023

ovitrif May 8, 2023

pachlava May 8, 2023

jostnes May 8, 2023

pachlava May 4, 2023

jostnes May 8, 2023

pachlava May 8, 2023 •

edited

ovitrif May 8, 2023

pachlava May 8, 2023

jostnes left a comment

jostnes May 8, 2023

jostnes May 8, 2023

jostnes May 8, 2023

pachlava commented May 8, 2023

jostnes left a comment

[UI Tests] Minor UI Tests code refactoring. #18349

[UI Tests] Minor UI Tests code refactoring. #18349

Conversation

pachlava commented May 2, 2023 • edited

Description

Testing

wpmobilebot commented May 2, 2023 • edited

wpmobilebot commented May 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pachlava May 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jostnes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pachlava commented May 8, 2023

jostnes left a comment

Choose a reason for hiding this comment

pachlava commented May 2, 2023 •

edited

wpmobilebot commented May 2, 2023 •

edited

wpmobilebot commented May 2, 2023 •

edited

pachlava May 8, 2023 •

edited