-
Notifications
You must be signed in to change notification settings - Fork 149
Fixes UI tests #817
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
Fixes UI tests #817
Conversation
appropriate button, and strips out whitespace between HTML tags.
Comments out two tests which are failing for other reasons and should be looked into separately.
rachelmcr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to strip the whitespace characters. Those are useful if we want to test the HTML view layout (proper list indentation there, etc) but they aren't really critical.
The changes I've suggested will fix the More Tag tests, at which point everything is passing and looks good to me.
|
|
||
| // Expects the format bar to be expanded. | ||
| let elementsQuery = app.scrollViews.otherElements | ||
| elementsQuery.buttons[elementStringIDs.mediaButton].swipeLeft() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This swipe is causing the blockquote issue in the More Tag tests. Because the toolbar is already swiped all the way to the right edge, when the test tries to perform this swipe it ends up tapping on the blockquote button on the left edge of the toolbar instead.
We can remove this line — the test will swipe as needed to get to the toolbar button it's trying to tap on (in this case, sourcecodeButton). I checked and without this line all the tests (including the More Tag tests) pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test will swipe as needed to get to the toolbar button it's trying to tap on
That is strange, but it isn't working like that on iPhone SE but works fine for iPhone 8
| } | ||
|
|
||
| /* | ||
| Commenting these out because they fails: Why is the more tag wrapped in a blockquote? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my note about the getHTMLContent() helper — this is a side effect of the swipe action added there, rather than a bug.
We can update the More Tag tests to the current expected output ("<p><!--more--></p>" and "<p>1</p><p><!--more--></p><p>2</p>", respectively) and with the change to that helper method these tests will pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some important info on this: the wrapping we're doing here (I mean the output we provide given the input) is not great. I was aware of this but failed to track this issue.
I think the outputs should be:
1: <!--more-->
2: <p>1</p><!--more><p>2</p>
I've now opened an issue to track this problem, so I'd consider leaving these disabled (and we'll turn them on when that issue is resolved).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, here's the issue I created: #818
I just reread this note; were you having trouble with this function not working as expected before? XCUITest should always scroll in scrollViews to the element you're trying to tap, without having to explicitly swipe. |
Changes helper function to scroll if the button isn't tappable.
|
@rachelmcr yes - this wasn't working for me. Also, we need to handle the case where the toolbar isn't expanded. The button is missing an a11y label. IMO though, we should just always expand the toolbar in the example app. For now, I updated the comment for accuracy, and added a check which only scrolls if the button isn't there. This fixed the blockquote thing, but because the behaviour will change in #818 I left it as-is for now. |
Yes, we store the expanded status in |
|
@frosty - will need to figure out a11y for the states then "more toolbar options" / "fewer toolbar options"? But regardless, IMO better to have one test that checks compressing / expanding rather than needing to expand it every test. |
diegoreymendez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
|
@diegoreymendez can you merge this please? <3 |
|
Awesome contribution @catehstn. Thank you! 👍 |
Fixed the getHTMLContent function - now scrolls the toolbar to tap the appropriate button.
Is whether or not the toolbar is expanded by default a device setting? Would probably enable that by default for the demo as it makes this easier.
Added
<p>and</p>tags to get tests to pass. If this behaviour changes, these tests will need to be updated. Comments out two tests that need more change (and are maybe surfacing bugs).Uncommented out the list tests, minor changes to get those working.
Stripping out all whitespace characters between tags as we should not IMO be testing that. Open to feedback here.
To test: Open example app, and run the tests. Confirm that they pass.