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

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

Merged
merged 3 commits into from
Nov 26, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions WordPress/UITestsFoundation/Screens/ReaderScreen.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,47 @@ public class ReaderScreen: ScreenObject {
)
}

public func openLastPost() {
getLastPost().tap()
}

public func openLastPostInSafari() {
getLastPost().buttons["More"].tap()
app.buttons["Visit"].tap()
}

public func getLastPost() -> XCUIElement {
guard let post = app.cells.lastMatch else { fatalError("ReaderScreen: No posts loaded") }
scrollDownUntilElementIsHittable(element: post)
return post
}

private func scrollDownUntilElementIsHittable(element: XCUIElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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

public func postContentEquals(_ expected: String) -> Bool {
let equalsPostContent = NSPredicate(format: "label == %@", expected)
let isPostContentEqual = app.staticTexts.element(matching: equalsPostContent).waitForIsHittable(timeout: 3)

return isPostContentEqual
}

public func dismissPost() {
let backButton = app.buttons["Back"]
let dismissButton = app.buttons["Dismiss"]

if dismissButton.isHittable {
dismissButton.tap()
} else if backButton.isHittable {
backButton.tap()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen in the tests if neither is hittable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

}

public static func isLoaded() -> Bool {
(try? ReaderScreen().isLoaded) ?? false
}
Expand Down
4 changes: 4 additions & 0 deletions WordPress/WordPress.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -2494,6 +2494,7 @@
E6F2788421BC1A4A008B4DB5 /* PlanFeature.swift in Sources */ = {isa = PBXBuildFile; fileRef = E6F2787E21BC1A49008B4DB5 /* PlanFeature.swift */; };
E6FACB1E1EC675E300284AC7 /* GravatarProfile.swift in Sources */ = {isa = PBXBuildFile; fileRef = E6FACB1D1EC675E300284AC7 /* GravatarProfile.swift */; };
E8DEE110E4BC3FA1974AB1BB /* Pods_WordPressTest.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = B921F5DD9A1F257C792EC225 /* Pods_WordPressTest.framework */; };
EAB10E4027487F5D000DA4C1 /* ReaderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = EAB10E3F27487F5D000DA4C1 /* ReaderTests.swift */; };
F10465142554260600655194 /* BindableTapGestureRecognizer.swift in Sources */ = {isa = PBXBuildFile; fileRef = F10465132554260600655194 /* BindableTapGestureRecognizer.swift */; };
F10D634F26F0B78E00E46CC7 /* Blog+Organization.swift in Sources */ = {isa = PBXBuildFile; fileRef = F10D634E26F0B78E00E46CC7 /* Blog+Organization.swift */; };
F10D635026F0B78E00E46CC7 /* Blog+Organization.swift in Sources */ = {isa = PBXBuildFile; fileRef = F10D634E26F0B78E00E46CC7 /* Blog+Organization.swift */; };
Expand Down Expand Up @@ -7341,6 +7342,7 @@
E6F2787E21BC1A49008B4DB5 /* PlanFeature.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PlanFeature.swift; sourceTree = "<group>"; };
E6FACB1D1EC675E300284AC7 /* GravatarProfile.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = GravatarProfile.swift; sourceTree = "<group>"; };
E850CD4B77CF21E683104B5A /* Pods-WordPressStatsWidgets.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-WordPressStatsWidgets.debug.xcconfig"; path = "../Pods/Target Support Files/Pods-WordPressStatsWidgets/Pods-WordPressStatsWidgets.debug.xcconfig"; sourceTree = "<group>"; };
EAB10E3F27487F5D000DA4C1 /* ReaderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ReaderTests.swift; sourceTree = "<group>"; };
EEF80689364FA9CAE10405E8 /* Pods-WordPressNotificationServiceExtension.release-alpha.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-WordPressNotificationServiceExtension.release-alpha.xcconfig"; path = "../Pods/Target Support Files/Pods-WordPressNotificationServiceExtension/Pods-WordPressNotificationServiceExtension.release-alpha.xcconfig"; sourceTree = "<group>"; };
EF379F0A70B6AC45330EE287 /* Pods-WordPressTest.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-WordPressTest.release.xcconfig"; path = "../Pods/Target Support Files/Pods-WordPressTest/Pods-WordPressTest.release.xcconfig"; sourceTree = "<group>"; };
F10465132554260600655194 /* BindableTapGestureRecognizer.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = BindableTapGestureRecognizer.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -13119,6 +13121,7 @@
BEA101B61FF13F0500CE5C7D /* Tests */ = {
isa = PBXGroup;
children = (
EAB10E3F27487F5D000DA4C1 /* ReaderTests.swift */,
6E5BA46826A59D620043A6F2 /* SupportScreenTests.swift */,
FF2716911CAAC87B0006E2D4 /* LoginTests.swift */,
CC7CB97222B1510900642EE9 /* SignupTests.swift */,
Expand Down Expand Up @@ -20725,6 +20728,7 @@
buildActionMask = 2147483647;
files = (
FFA0B7D71CAC1F9F00533B9D /* MainNavigationTests.swift in Sources */,
EAB10E4027487F5D000DA4C1 /* ReaderTests.swift in Sources */,
BED4D8301FF11DEF00A11345 /* EditorAztecTests.swift in Sources */,
3F2F856326FAF612000FCDA5 /* EditorGutenbergTests.swift in Sources */,
FF2716921CAAC87B0006E2D4 /* LoginTests.swift in Sources */,
Expand Down
36 changes: 36 additions & 0 deletions WordPress/WordPressUITests/Tests/ReaderTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import UITestsFoundation
import XCTest

class ReaderTests: XCTestCase {
private var readerScreen: ReaderScreen!

override func setUpWithError() throws {
setUpTestSuite()

_ = try LoginFlow.loginIfNeeded(siteUrl: WPUITestCredentials.testWPcomSiteAddress, email: WPUITestCredentials.testWPcomUserEmail, password: WPUITestCredentials.testWPcomPassword)
tiagomar marked this conversation as resolved.
Show resolved Hide resolved
readerScreen = try EditorFlow
.goToMySiteScreen()
.tabBar.goToReaderScreen()
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tiagomar tiagomar Nov 26, 2021

Choose a reason for hiding this comment

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

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.

readerScreen.dismissPost()
}
try LoginFlow.logoutIfNeeded()
try super.tearDownWithError()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tiagomar tiagomar Nov 25, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

let expectedPostContent = "Aenean vehicula nunc in sapien rutrum, nec vehicula enim iaculis. Aenean vehicula nunc in sapien rutrum, nec vehicula enim iaculis. Proin dictum non ligula aliquam varius. Nam ornare accumsan ante, sollicitudin bibendum erat bibendum nec. Aenean vehicula nunc in sapien rutrum, nec vehicula enim iaculis."

func testViewPost() {
readerScreen.openLastPost()
XCTAssert(readerScreen.postContentEquals(expectedPostContent))
}

func testViewPostInSafari() {
readerScreen.openLastPostInSafari()
XCTAssert(readerScreen.postContentEquals(expectedPostContent))
}
}