Add UI tests to check the ability to scroll reader content and interact with the items #17536

tiagomar · 2021-11-23T14:54:44Z

One of the identified critical flows to be automated was to open the Reader section, scroll content, open and close selections. This PR adds tests to cover the scenario.

To test:
ReaderTests must pass on CI.

Regression Notes

Potential unintended areas of impact
None. The changes would only impact ReaderTests.
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
N/A

PR submission checklist:

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

peril-wordpress-mobile · 2021-11-23T14:54:48Z

You can trigger an installable build for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-11-23T15:15:25Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

mokagio

CI is happy, and the tests passed locally, too 🎉

I left a few questions and suggestions. Once those are addressed, this will be ready to merge as far as I'm concerned.

mokagio · 2021-11-24T04:51:27Z

WordPress/UITestsFoundation/Screens/ReaderScreen.swift

@@ -20,6 +20,50 @@ public class ReaderScreen: ScreenObject {
        )
    }

+    public func openTheLastPostInApp() {


Nitpick: Function names don't usually have articles in them. I don't see it spelled out clearly in the Swift API Guidelines and, thinking about it, I never really read this anywhere, but, based on all the code I've read over time, I think my assertion is accurate.

Suggested change

public func openTheLastPostInApp() {

public func openLastPostInApp() {

Also, I think it's safe to omit "in app" from the name. I'm guessing you did it to differentiate it from the method below that uses Safari. But, IMHO, if nothing else is specified it's safe to assume the action will occur in the app.

Yeah, the idea was really to differentiate it. I was actually a bit uncomfortable with "InApp" because the "InSafari" is somewhat still in app. =P

I'll get rid of the articles. =)

mokagio · 2021-11-24T04:54:45Z

WordPress/UITestsFoundation/Screens/ReaderScreen.swift

+
+    public func getTheLastPost() -> XCUIElement {
+        let postPosition = app.cells.count - 1
+        let post = app.cells.element(boundBy: postPosition)


Out of curiosity, did you try the lastMatch helper here? It should return the same value.

WordPress-iOS/WordPress/UITestsFoundation/XCUIElementQuery+Utils.swift

Lines 6 to 8 in 01ccb69

var lastMatch: XCUIElement? {

return self.allElementsBoundByIndex.last

}

I haven't tried it, probably because I started writing a more generic function to get the Nth post but then I realized I was doing premature optimization and also affecting the test readability. I'll use lastMatch. =)

Yeah, no stress. I was just curious.

There are good utilities in our XCUITestHelpers library, but one needs to know about them first, and we don't have an easy way to discover them. I guess it's just like with any other library: We get familiar with them by using them and we can all help each other out by sharing suggestions 😄

By the way: +1 for avoiding premature optimization. At the same time, if you stumble upon something that's a bit over-engineered for the current use case but looks general and something that could be useful in other tests feel free to implement it that way and extracting it into XCUITestHelpers as a followup 👍

mokagio · 2021-11-24T04:55:48Z

WordPress/UITestsFoundation/Screens/ReaderScreen.swift

+        while !element.waitForIsHittable(timeout: 3) && loopCount < 10 {
+            loopCount += 1
+            app.swipeUp(velocity: .fast)
+            print(loopCount)


This looks like a debug statement. Did you intend to leave it in here? If not:

Suggested change

print(loopCount)

🤦 Yeah, don't tell my mother.

mokagio · 2021-11-24T04:57:25Z

WordPress/UITestsFoundation/Screens/ReaderScreen.swift

+            dismissButton.tap()
+        } else if backButton.isHittable {
+            backButton.tap()
+        }


What would happen in the tests if neither is hittable?

The dismissPost() is called only in tearDownWithError(). None of them being hittable will only happen if the tests fail prematurely even before opening the post and tearDownWithError() would continue its flow normally.

mokagio · 2021-11-24T05:01:07Z

WordPress/WordPressUITests/Tests/ReaderTests.swift

+            readerScreen.dismissPost()
+        }
+        try LoginFlow.logoutIfNeeded()
+        try super.tearDownWithError()


It should not be necessary to call super here. Did you add this because of some error in the tests?

No, I have just followed what all the other tests were doing. I did try removing the super now but it went into a recursive call loop.

It runs just fine when removing the whole line but I would be more comfortable removing it if I knew why it was added in first place.

Ouch! My suggestions was not clear, sorry 😳

"to call super" should have been "to call super.tearDownWithError() or "to call the super implementation".

If you updated to

- try super.tearDownWithError() + try tearDownWithError()

it will indeed got into an infinite loop. ♻️

No, I have just followed what all the other tests were doing.

Fair enough. Those methods are (should be?) empty in XCTestCase so it doesn't hurt to call them. It simply adds unnecessary code to the test.

Feel free to ignore this. I plan to audit the test suite for this kind of stuff during my HACK week. 👍 #17569

WordPress/UITestsFoundation/Screens/ReaderScreen.swift

mokagio · 2021-11-24T05:12:48Z

WordPress/UITestsFoundation/Screens/ReaderScreen.swift

+        return post
+    }
+
+    private func scrollDownUntilElementIsHittable(element: XCUIElement) {


This is a nice method. Probably better than the scrollIntoView(within:, threshold:) we already have, because it doesn't rely on already knowing the scrollable container element.

I might steal it in the future 😄

I assume (I didn't test this) that this method is designed to scroll and wait for hittability of the last element only, because swiping up with fast velocity will probably will make it not suitable for the ones in the middle; and because it's called bygetLastPost().

If this is so, what do you think of renaming the method to reflect this?

mokagio

Thank you for following up @tiagomar.

pachlava

I started from executing the tests on iPhone 12, and they passed. Nice job @tiagomar, the code looks solid to me 👍.

I left some notes, but none of them are blockers. Started review earlier today, but actually finished after Gio already approved the PR. Two approvals are better than one 😄

pachlava · 2021-11-25T15:55:11Z

WordPress/UITestsFoundation/Screens/ReaderScreen.swift

+        return post
+    }
+
+    private func scrollDownUntilElementIsHittable(element: XCUIElement) {


I assume (I didn't test this) that this method is designed to scroll and wait for hittability of the last element only, because swiping up with fast velocity will probably will make it not suitable for the ones in the middle; and because it's called bygetLastPost().

If this is so, what do you think of renaming the method to reflect this?

pachlava · 2021-11-25T16:19:14Z

WordPress/WordPressUITests/Tests/ReaderTests.swift

+
+    override func tearDownWithError() throws {
+        takeScreenshotOfFailedTest()
+        if readerScreen != nil && !TabNavComponent.isVisible() {


Just thinking aloud: as long as dismissPost() is used only here, it's fine to have this 'fuse' here, but once it will be used in the other places too (like inside the tests), I think this check can be moved to the dismissPost() method itself.

If this is so, what do you think of renaming the method to reflect this?

The idea was to have it more generic but you are right about the .fast. It wouldn't make much difference in this case though, because the posts are mocked and we have only 3 of them.

WordPress/WordPressUITests/Tests/ReaderTests.swift

pachlava · 2021-11-25T21:53:32Z

WordPress/UITestsFoundation/Screens/ReaderScreen.swift

+        if dismissButton.isHittable {
+            dismissButton.tap()
+        } else if backButton.isHittable {
+            backButton.tap()
+        }


Currently, the method assumes that the reader post will be either opened in a usual way (and then Back button should be used), or that it will be opened via More>Visit from reader list view. But there's also a third option: if it's opened in Safari from the post view (by clicking the globe button). In that case, it firstly needed to tap Dismiss and then Back. I assume that changing else if to just if would allow to also support this scenario.

Good point!

Add reader ui critical tests

25c615f

tiagomar added the Testing Unit and UI Tests and Tooling label Nov 23, 2021

tiagomar requested review from pachlava, jostnes and mokagio November 23, 2021 19:23

tiagomar marked this pull request as ready for review November 23, 2021 19:24

mokagio reviewed Nov 24, 2021

View reviewed changes

Readability improvements

8b48f5a

tiagomar requested a review from mokagio November 25, 2021 17:11

mokagio approved these changes Nov 25, 2021

View reviewed changes

pachlava approved these changes Nov 25, 2021

View reviewed changes

Readability improvement to dismissPost

40ba38f

tiagomar merged commit 8ceb016 into develop Nov 26, 2021

tiagomar deleted the reader-ui-tests-clean branch November 26, 2021 15:09

pachlava mentioned this pull request Nov 30, 2021

Add UI test to verify the site stats are being loaded properly #17593

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UI tests to check the ability to scroll reader content and interact with the items #17536

Add UI tests to check the ability to scroll reader content and interact with the items #17536

tiagomar commented Nov 23, 2021

peril-wordpress-mobile bot commented Nov 23, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Nov 23, 2021 •

edited

Loading

mokagio left a comment

mokagio Nov 24, 2021

tiagomar Nov 25, 2021

mokagio Nov 24, 2021

tiagomar Nov 25, 2021

mokagio Nov 25, 2021

mokagio Nov 24, 2021

tiagomar Nov 25, 2021

mokagio Nov 24, 2021

tiagomar Nov 25, 2021

mokagio Nov 25, 2021

mokagio Nov 24, 2021

tiagomar Nov 25, 2021 •

edited

Loading

mokagio Nov 25, 2021

mokagio Nov 24, 2021

pachlava Nov 25, 2021

mokagio left a comment

pachlava left a comment

pachlava Nov 25, 2021

pachlava Nov 25, 2021

tiagomar Nov 26, 2021 •

edited

Loading

pachlava Nov 25, 2021

tiagomar Nov 26, 2021

	public func openTheLastPostInApp() {
	public func openLastPostInApp() {

	var lastMatch: XCUIElement? {
	return self.allElementsBoundByIndex.last
	}

Add UI tests to check the ability to scroll reader content and interact with the items #17536

Add UI tests to check the ability to scroll reader content and interact with the items #17536

Conversation

tiagomar commented Nov 23, 2021

Regression Notes

peril-wordpress-mobile bot commented Nov 23, 2021 • edited Loading

peril-wordpress-mobile bot commented Nov 23, 2021 • edited Loading

mokagio 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

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

tiagomar Nov 25, 2021 • edited Loading

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

mokagio left a comment

Choose a reason for hiding this comment

pachlava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagomar Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Nov 23, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Nov 23, 2021 •

edited

Loading

tiagomar Nov 25, 2021 •

edited

Loading

tiagomar Nov 26, 2021 •

edited

Loading