Add UI tests for high priority issues #819

brbrr · 2017-11-10T10:53:41Z

Will incrementally add UI tests to existing High priority issues

Fixed: #839

rachelmcr

Nice work on these so far. I have some thoughts/questions about the line height tests, but I like the overall approach you're taking with these.

rachelmcr · 2017-11-10T15:09:23Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+    }
+
+    // Gihtub issue #393
+    func testLongTitle() {


This test only passes on smaller screens — it fails on larger screens (I checked on an iPad Pro) because the text isn't long enough to wrap there. We should make sure the test accounts for different screen sizes and passes on any device.

rachelmcr · 2017-11-10T15:14:57Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+
+        titleTextView.typeText("very very very very very very long title in a galaxy not so far away")
+        let twoLineTitleHeight = Int(titleTextView.frame.height)
+        XCTAssert(twoLineTitleHeight - oneLineTitleHeight == titleLineHeight )


Instead of setting titleLineHeight (fragile due to device settings or design changes) and using it in this calculation, what about using oneLineTitleHeight and asserting that twoLineTitleHeight is twice that? (Or just that twoLineTitleHeight is a multiple of oneLineTitleHeight if we want to confirm that the title area expands.)

I totally agree with you and I was thinking about that one. The thing is that oneLineTitleHeight is about 38px (due to underline and some padding), but every other line is 22px. So its quite complicated to confirm that in a nice way. We can confirm that it's just bigger, but I don't think its good approach either.

Ah right, I see what you mean. I agree that it'd be best to confirm a specific change in line height, so it might be best to stick with this approach. I'll keep thinking about it. (In the future, if we implement visual regression tests for Aztec we could confirm in the UI test that it's just bigger and use the visual regression test to make sure the entire title is visible. I wouldn't rely on that until we start implementing those visual tests, though.)

rachelmcr · 2017-11-10T15:21:11Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+//  Created by brbrr on 11/10/17.
+//  Copyright © 2017 Automattic Inc. All rights reserved.
+//
+


You can remove these default header comments from the top of the file.

rachelmcr · 2017-11-10T15:26:39Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+        //        XCTAssert(oneLineTitleHeight < twoLineTitleHeight)
+    }
+
+    func testNewlinesInTitle() {


I know this is the current behavior, but should Aztec allow new lines in the title field? cc @diegoreymendez for your thoughts on this.

I don't think we really need this test, given that this isn't the intended behavior. See: #821

rachelmcr · 2017-11-10T15:27:46Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+        super.tearDown()
+    }
+
+    // Gihtub issue #393


I'd go ahead and add the full link to the issue here, to make it easier to find. (Also, just a small typo in "Gihtub")

rachelmcr · 2017-11-10T15:29:02Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+        //        XCTAssert(twoLineTitleHeight < threeLineTitleHeight)
+    }
+
+    func testInfinitLoopOnAssetDownload() {


What issue does this test? I think it's worth adding comments with links to all the issues we're testing, for reference later on. (Also, just a small note that there's a typo - "Infinit" should be "Infinite")

Added GH issue ✅

rachelmcr · 2017-11-10T15:32:07Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+        switchContentView()
+        gotoRootPage()
+
+        let editorDemoButton = app.tables/*@START_MENU_TOKEN@*/.staticTexts["Empty Editor Demo"]/*[[".cells.staticTexts[\"Empty Editor Demo\"]",".staticTexts[\"Empty Editor Demo\"]"],[[[-1,1],[-1,0]]],[0]]@END_MENU_TOKEN@*/


You can use staticTexts[elementStringIDs.emptyDemo] here

wow! That is some fancy XCode recording stuff holding few locator values. I'll replace it with the single value you suggest.

Oh, I should clarify that I was thinking of replacing staticTexts["Empty Editor Demo"] with staticTexts[elementStringIDs.emptyDemo]. We can remove the comments here and just use let editorDemoButton = app.tables.staticTexts[elementStringIDs.emptyDemo].

rachelmcr · 2017-11-10T15:33:36Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+        gotoRootPage()
+
+        let editorDemoButton = app.tables/*@START_MENU_TOKEN@*/.staticTexts["Empty Editor Demo"]/*[[".cells.staticTexts[\"Empty Editor Demo\"]",".staticTexts[\"Empty Editor Demo\"]"],[[[-1,1],[-1,0]]],[0]]@END_MENU_TOKEN@*/
+        XCTAssert(editorDemoButton.exists, "Editor button not hittable. Arew you on right page?")


Just a minor suggested edit:

"Editor button not hittable. Are you on the right page?"

rachelmcr · 2017-11-10T15:47:17Z

Example/AztecUITests/XCTest+Extensions.swift

 extension XCTest {
+    /**
+     Common method to type in different text fields
+     */


Nice! I like these updates you've made to the helper functions.

diegoreymendez · 2017-11-12T17:01:32Z

@brbrr, @catehstn, @rachelmcr - While reviewing these tests I can't really help it but notice we're devoting a lot of effort into UI testing the library's example App.

While this is good for Aztec as a library, most of the UI code is not directly affecting WPiOS (since Aztec as a library only provides the editor view).

I feel like it may be a good idea to start pushing at least part of these into WPiOS, instead or in addition to Aztec's example App.

Thoughts?

brbrr · 2017-11-14T13:30:09Z

@diegoreymendez Generally I agree with you. Our (mine w/ @rachelmcr) plan was to cover all High priority issues (which are worth to cover w/ UI tests) here since test flows are mostly short. So we don't need to bother with login flow etc.

After that is done (and maybe some other simple formatting tests) we will switch to WP apps. Is it sounds reasonable to you?

And maybe we should move this discussion elsewhere (call or P2)

diegoreymendez · 2017-11-14T17:29:42Z

Just to clarify though. I believe that the work you're doing here is great for several different reasons.

Ideally we'd have both the library and the app covered by a similar set of tests.

Thanks for taking care, everyone!

…rdpress-mobile/AztecEditor-iOS into add/ui-tests-for-high-priority-issues

brbrr · 2017-11-24T12:53:51Z

@rachelmcr may I ask your attention to this one?

rachelmcr

I love the page object improvements!

The tests are working great for me on smaller devices, so my comments at this point are mainly focused on getting the tests working on larger devices.

rachelmcr · 2017-11-24T15:57:43Z

Example/AztecUITests/AztecUITests.swift

+//        tablesQuery.staticTexts[elementStringIDs.emptyDemo].tap()
+//
+//        let richTextField = app.textViews[elementStringIDs.richTextField]
+//        richTextField.tap()


I'd go ahead and remove this commented-out section.

rachelmcr · 2017-11-24T16:00:39Z

Example/AztecUITests/Pages/BlogsPage.swift

+        expandOptionsSctrip()
+    }
+
+    func expandOptionsSctrip() -> Void {


On larger devices (e.g. iPad Pro) this function is toggling the extended toolbar open/closed rather than always expanding it. We should check the status of the toolbar before tapping — otherwise the expanded toolbar is toggled off in every other test.

If it isn't currently possible to check the status, we should open an issue to address that as it's also an accessibility issue. (VoiceOver users need to be aware of the toolbar status so they can expand it if needed, too.)

Created the ticket for that: #837

For now, how about checking to see if the HTML button exists before toggling the extended toolbar? I'm thinking of something like this:

let expandButton = app.children(matching: .window).element(boundBy: 1).children(matching: .other).element.children(matching: .other).element.children(matching: .other).element.children(matching: .button).element let htmlButton = app.scrollViews.otherElements.buttons[elementStringIDs.sourcecodeButton] if expandButton.exists && expandButton.isHittable && !htmlButton.exists { expandButton.tap() }

That way we don't break the existing tests on iPads, and we can improve the check later once that issue is resolved.

That's nice solution, thanks!

rachelmcr · 2017-11-24T16:08:15Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+    }
+
+    // Github issue https://github.com/wordpress-mobile/AztecEditor-iOS/issues/385
+    func testLongTitle() {


This test is still failing on larger devices (e.g. iPad Pro). If there isn't a way to get it to work for both large and small devices, could we restrict it to only run when testing smaller devices?

I don't think its possible to tag which tests to run with current test runner (as it's done in wp-e2e-tests). The only solution I see is to move such tests to separate files and run them only for small screens. And in general, I think it would be better to address this issue once we'll have them running on multiple devices. I've created the ticket for that: #839

I looked around and found these hints and tips that include conditional code for testing on iPhone v. iPad. I updated the example shown there and got it to work in this test like this:

if (app.windows.element(boundBy: 0).horizontalSizeClass == .compact || app.windows.element(boundBy: 0).verticalSizeClass == .compact) { titleTextView.typeText("very very very very very very long title in a galaxy not so far away") } else { titleTextView.typeText("very very very very very very long title in a galaxy not so far away very very very very very very long title in a galaxy not so far away") }

rachelmcr · 2017-11-24T16:12:16Z

Example/AztecUITests/HighPriorityIssuesTests.swift

+        //        XCTAssert(oneLineTitleHeight < twoLineTitleHeight)
+    }
+
+    func testNewlinesInTitle() {


I don't think we really need this test, given that this isn't the intended behavior. See: #821

rachelmcr · 2017-11-24T16:13:28Z

Example/AztecUITests/XCTest+Extensions.swift

+//        let app = XCUIApplication()
+//        let richTextField = app.textViews[elementStringIDs.richTextField]
+//
+//        richTextField.typeText(text)


I'd go ahead and remove this commented-out section.

rachelmcr · 2017-11-30T18:26:58Z

The changes look good to me. It looks like testCopyPasteCrash failed on Travis CI — I'm not sure what's going on there; I haven't had any trouble with it on local runs.

diegoreymendez · 2017-12-15T18:05:47Z

Just a reminder to set the appropriate milestone when closing a PR or issue. Thanks!

brbrr added 2 commits November 10, 2017 12:43

Add AztecUITests to AztecExample scheme

3718e0a

Add tests for #393

42c7333

brbrr requested a review from rachelmcr November 10, 2017 11:07

brbrr added the [Type] Automated Testing label Nov 10, 2017

brbrr added 3 commits November 10, 2017 14:29

Add #675 UI test and some refactoring

75c88f5

Add #465 UI test

85422c0

Make tests work on iPhone SE

0f9fde1

rachelmcr reviewed Nov 10, 2017

View reviewed changes

Updates after PR review

55398ef

Add tests #771 & #768

84d4b27

brbrr added 12 commits November 15, 2017 21:42

minor changes

d71268e

Merge branch 'develop' into add/ui-tests-for-high-priority-issues

fbfb2f8

dont do missclick when mediaButton is not Hittable

eebf314

Reset simulator state before test run

aa209f9

Check programatically whenever options strip is in expanded postion

818936c

minor changes, some work on PageObject refactoring

01c6ba2

Merge branch 'develop' into add/ui-tests-for-high-priority-issues

0f557ab

fix project structure

f822bc4

Merge branch 'add/ui-tests-for-high-priority-issues' of github.com:wo…

69b96f7

…rdpress-mobile/AztecEditor-iOS into add/ui-tests-for-high-priority-issues

tweaking failed test

ee772cf

another tweak for failed test

7496657

tweak for swiping issue on iPhone SE

7e749be

rachelmcr suggested changes Nov 24, 2017

View reviewed changes

brbrr added 3 commits November 27, 2017 10:58

clean-up

cf32c02

Add some reliability to tests on bigger screens

19d2314

Tests passing on iPads 🎉

1563123

brbrr mentioned this pull request Nov 30, 2017

Make UI test to pass on bigger devices (iPad PRO) #839

Closed

brbrr changed the title ~~[WIP] Add UI tests for high priority issues~~ Add UI tests for high priority issues Nov 30, 2017

rachelmcr approved these changes Nov 30, 2017

View reviewed changes

brbrr merged commit aec651b into develop Dec 1, 2017

brbrr deleted the add/ui-tests-for-high-priority-issues branch December 14, 2017 03:49

diegoreymendez added this to the 1.0.0-beta.16 milestone Dec 15, 2017

Add UI tests for high priority issues #819

Add UI tests for high priority issues #819

Uh oh!

Conversation

brbrr commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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!

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!

diegoreymendez commented Nov 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brbrr commented Nov 14, 2017

Uh oh!

diegoreymendez commented Nov 14, 2017

Uh oh!

brbrr commented Nov 24, 2017

Uh oh!

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

brbrr Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

brbrr Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

brbrr commented Nov 10, 2017 •

edited

Loading

diegoreymendez commented Nov 12, 2017 •

edited

Loading

brbrr Nov 27, 2017 •

edited

Loading

brbrr Nov 27, 2017 •

edited

Loading