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 for simple text formatting #423

Merged
merged 7 commits into from Apr 19, 2017

Conversation

rachelmcr
Copy link
Member

This PR adds a suite of UI tests for simple text formatting in the Aztec demo app.

They test each of the text formatting buttons in the toolbar (including the hr and more tags). The tests for lists are currently disabled, pending lists being fully implemented in Aztec.

Pinging @hoverduck and @catehstn for a review. :)

@rachelmcr
Copy link
Member Author

Also, h/t @SergioEstevao for the accessibility improvements. I used the model from wordpress-mobile/WordPress-iOS#6967 to add accessibility labels and IDs to the Aztec demo app, for use in these tests.

@diegoreymendez diegoreymendez self-requested a review Apr 12, 2017
@diegoreymendez
Copy link
Contributor

Wow, nice contribution @rachelmcr! Really appreciated!

I'm adding myself as reviewer mostly to point out a few minor standardization changes I've spotted.

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

This is a great work!

I pointed out some minor style differences to make the code standard across our codebase. Check out the inline comments for more info on this.

The bigger issue I found is that all tests are all failing for me due to an issue with the auto-correction text prediction in iOS messing up one of the steps.

Check out this comment for a working fix for it (tested and working fine here).

Here's a video showing the problem I'm seeing.

app.launch()

let tablesQuery = app.tables
tablesQuery.staticTexts[ elementStringIDs.emptyDemo ].tap()
Copy link
Contributor

Choose a reason for hiding this comment

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

To standardize with the rest of the existing code, it would be good to remove the extra spacing after [ and before ] - I'm mentioning it here, but it'd apply to all of the places where we have these characters.

This line would become:

tablesQuery.staticTexts[elementStringIDs.emptyDemo].tap()

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I've fixed these.

app.buttons[ elementStringIDs.sourcecodeButton ].tap()

htmlContentTextView = app.textViews[ elementStringIDs.htmlTextField ]
let text : String = htmlContentTextView.value as! String
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, minor note to standardize with the rest of the code. When declaring variables we're attaching the : to the variable name.

This case would become:

let text: String = htmlContentTextView.value as! String

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! These are fixed, too.

func enterAndSelectText(text: String) -> Void {
let app = XCUIApplication()

self.typeText(text)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for self. in this line, and the next one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, thanks!


self.typeText(text)
self.press(forDuration: 1.2)
app.menuItems.element(boundBy: 1).tap()
Copy link
Contributor

Choose a reason for hiding this comment

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

The UI tests are failing for me, and it seems to be related with using an index for picking the right menu-item to press (which I imagine is "Select All").

One way I found to work around this issue was to replace this line with exactly:

app.menuItems["Select All"].tap()

Copy link
Member Author

Choose a reason for hiding this comment

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

Argh, text prediction. I was seeing those failures earlier but after some updates stopped seeing them (even with prediction enabled), so I thought it wasn't an issue anymore.

I was trying to avoid relying on the string "Select All", so the tests will work in other languages. I was playing with pasting the text instead of typing, to avoid text prediction issues, so I'm going to try that out and see how it goes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree relying on text prediction is not the best - also need to consider what if it's not there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is to turn off autocorrect at the start of the tests.

I don't think XCUI allows us to leave the app and go to settings: http://osxdaily.com/2015/01/06/disable-auto-correct-ios/

But apparently it is a property you can change on a text input: http://stackoverflow.com/questions/11789594/how-to-programmatically-turn-off-auto-correction-in-iphone-sdk

Copy link
Member Author

Choose a reason for hiding this comment

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

Although we can use the autocorrectionType property to change the UITextView behavior itself, it doesn't look like we can change that property in the tests only (at least in a reasonably simple/straightforward way, as far as I can tell).

I'll go ahead with pasting the text for now and will revisit if I work out a way to disable autocorrect instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I had a look into this and basically we check for the launch flag, and in a debug-testing mode, we turn it off.
@diegoreymendez what do you think? Numbers are OK here, but for proper coverage I think we have to find a way to disable autocorrect.

@astralbodies
Copy link
Contributor

tenor

Copy link
Contributor

@catehstn catehstn left a comment

Choose a reason for hiding this comment

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

This is fantastic!! Super happy about this - love the tests and the a11y improvements. Thanks Rachel!

}

func testSimpleItalicText() {
richContentTextView = app.textViews[elementStringIDs.richTextField]
Copy link
Contributor

Choose a reason for hiding this comment

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

this shows up in multiple places: I would add this to a helper eg enterTextInField().

Copy link
Contributor

Choose a reason for hiding this comment

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

also good because then whatever happens to stop autocorrect only happens in one helper (with explanation).

app.scrollViews.otherElements.buttons[elementStringIDs.italicButton].tap()
app.buttons[elementStringIDs.sourcecodeButton].tap()

htmlContentTextView = app.textViews[elementStringIDs.htmlTextField]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could also use another helper here which is getHtmlContent() returning a string.

}

// Enable this test after unordered lists are fully implemented
func testSimpleUnorderedListText() {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO for lists need multiple element tests also.

Copy link
Member Author

Choose a reason for hiding this comment

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

Per our Slack conversation, we'll revisit/expand the tests for lists once lists are fully implemented and changes in visual mode affect the HTML (#150). I'll leave the list tests as-is for now.

}

// Enable this test after ordered lists are fully implemented
func testSimpleOrderedListText() {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

// Copy link to be auto-filled in URL field
UIPasteboard.general.string = "https://wordpress.com/"
app.scrollViews.otherElements.buttons[elementStringIDs.linkButton].tap()
app.alerts.buttons.element(boundBy: 3).tap()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this boundBy: 3? does it not have an a11y label?

XCTAssertEqual(expected, text)
}

func testHorizontalRuler() {
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO should have a test for this that has text either side also.

XCTAssertEqual(expected, text)
}

func testMoreTag() {
Copy link
Contributor

Choose a reason for hiding this comment

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

same for this one.

XCTAssertEqual(expected, text)
}

func testHeadingOneText() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think headings also need tests that show that the heading style ends after tapping enter.

Copy link
Contributor

Choose a reason for hiding this comment

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

and that it isn't applied to text prior to the headings. and that two different heading types can be next to each other. probably enough there that it's worth making a separate file of tests for headings.

This would be fine as a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. My plan for this PR was to implement the most basic tests for adding the correct style to text, but we should add the header tests you've outlined as well.

We should also have similar tests for blockquotes and lists (to test that the style ends when expected and is only applied to the expected text), as well as combining styles appropriately.

I've made a note to revisit these in a future PR.

Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

Just wanted to leave a confirmation that my comments have been addressed appropriately.

Of course, this approval is just partial (there are still comments by @catehstn).

@rachelmcr
Copy link
Member Author

I updated the tests based on the feedback so far. A few notes:

  • After researching different methods for dealing with autocorrect/autocomplete, I switched to typing in numbers instead of letters/words. Numbers aren't autocorrected (yay!) so we avoid autocorrect without having to switch to pasting in the text. (I kept to one number at a time because with multiple numbers they were sometimes entered in the wrong order when running the tests.)
  • I also split up entering and selecting text in the helper functions. This should set us up well for testing a variety of scenarios where we focus on the rich text view, type some text, interact with the toolbar, and then continue typing (as seen in the added horizontal ruler and more tag tests).
  • The next step will be adding more complex tests that include both unformatted and formatted text in the rich text view, to make sure that the formatting is enabled/disabled as expected while interacting with the toolbar buttons.

@catehstn please let me know if this addresses your earlier feedback and if you see any other issues with these changes. Thanks!

Copy link
Contributor

@catehstn catehstn left a comment

Choose a reason for hiding this comment

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

I ran and the list ones are failing. I think comment them out and we can uncomment them when that functionality is there.

Also if you need to pause to insert multiple things, you could try waitForElementToAppear.

LGTM! Thanks :)

@diegoreymendez
Copy link
Contributor

@rachelmcr - Considering both reviewers gave approval: is this ready for merging?

I'm particularly interested in getting this included before the closed beta next week.

@hoverduck
Copy link

FWIW I also reviewed and give the 👍 from a general testing perspective. I haven't been able to get the project to build to actually run the tests yet, but that's an unrelated problem. /shrug

@rachelmcr rachelmcr merged commit 6c65e61 into develop Apr 19, 2017
@rachelmcr rachelmcr deleted the add/uitests-simple-text-formatting branch Apr 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants