Skip to content

Conversation

@rachelmcr
Copy link
Contributor

@rachelmcr rachelmcr commented Nov 10, 2017

  1. Adds UI tests for high priority issues Image upload crash #306 and Crash report wpandroid alpha-42: Undo while uploading image crashes app #299.
  2. Adds more UI tests for mixed formatting.
  3. Strips whitespace between HTML tags when verifying the HTML output (to match the iOS test behavior in Fixes UI tests AztecEditor-iOS#817).

To Test

Build and run the instrumentation tests, e.g. with ./gradlew cAT (see README for details).

Note that testRetainParagraphFormatting currently fails but will be resolved by #483.

.insertMedia()

// Must wait for simulated upload
Thread.sleep(10000)
Copy link

Choose a reason for hiding this comment

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

10s is quite a large delay. There no way to dynamically check if the upload is done?

Copy link
Contributor Author

@rachelmcr rachelmcr Nov 13, 2017

Choose a reason for hiding this comment

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

I was using the same general approach that was used in the previously added image tests with that sleep, but I can take another look to see what options there might be to dynamically check the upload status.

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 simulated upload in the demo app takes 10s, so I'm going to leave this as-is for our Aztec tests. I'll look at a more dynamic approach for the WordPress app tests.


for (i in 1..3) {
text += text
}
Copy link

Choose a reason for hiding this comment

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

Is this for cycle is needed? I think a simple string with multiple \n would make the test easier to read.

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 thought. I like the for loop for keeping things clean when we need a really long string, but I can probably just use a single string here.

@Test
fun testQuotedListFormatting() {
var text = "some text\n"
val regex = Regex("<blockquote>\\s+<ul>[\\S\\s]+</ul>\\s+</blockquote>")
Copy link

Choose a reason for hiding this comment

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

I'm not aware how Regex is working in Kotlin/Java but I've able to make it work after removing escaped \:
<blockquote>\s+<ul>[\S\s]+<\/ul>\s+<\/blockquote>

was matched:

<blockquote>
  <ul>
    <li>Wee</li>
    <li>Add</li>
    <li>Xox</li>
  </ul>
</blockquote>

Tested here: https://regexr.com/

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 backslashes need to be escaped per the pattern syntax. I'm not sure why the escaped version wouldn't work for you (although I'm also looking into stripping whitespace characters between HTML tags in our matchers for these tests, which will allow us to remove some of the regex here).

Copy link

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

I like the cleanness of your tests and changes, and everything looks nice and smooth! Good job 👍

@maxme maxme self-assigned this Nov 15, 2017
@rachelmcr rachelmcr changed the title [WIP] Add high priority and mixed formatting UI tests Add high priority and mixed formatting UI tests Nov 16, 2017
@rachelmcr rachelmcr requested a review from maxme November 16, 2017 11:14
@rachelmcr
Copy link
Contributor Author

Ok, I think the tests in this PR are complete. @maxme could you review this as well?

I'm going to look into adding some better link tests but I'll do that in a followup PR so I don't delay getting these high priority tests merged.

@maxme
Copy link
Contributor

maxme commented Nov 16, 2017

Test looks good, but do you mind implementing describeMismatchSafely in the Matchers (or maybe use TypeSafeDiagnosingMatcher, I'm not familiar with Harmcrest) to change the output of failing tests? It's hard to read the current output with the full view dump:

AssertionFailedWithCauseError: 'EditText matches hello world' doesn't match the selected view.
Expected: EditText matches hello world
Got: "SourceViewEditText{id=2131231019, res-name=source, visibility=VISIBLE, width=768, height=928, has-focus=true, has-focusable=true, has-window-focus=true, is-clickable=true, is-enabled=true, is-focused=true, is-focusable=true, is-layout-requested=false, is-selected=false, layout-params=android.widget.FrameLayout$LayoutParams@f80d919, tag=null, root-is-layout-requested=false, has-input-connection=true, editor-info=[inputType=0xa0001 imeOptions=0x58000005 privateImeOptions=null actionLabel=null actionId=0 initialSelStart=0 initialSelEnd=0 initialCapsMode=0x0 hintText=Codex is the source label=null packageName=null fieldId=0 fieldName=null extras=null hintLocales=null contentMimeTypes=null ], x=0.0, y=0.0, text=bye bye world, hint=Codex is the source, input-type=655361, ime-target=true, has-links=false}"

It would be great if we could get this instead:

AssertionFailedWithCauseError: 'EditText matches hello world' doesn't match the selected view.
Expected: "hello world"
Got: "bye bye world"

I can merge that PR and fill a ticket if you prefer.

@rachelmcr
Copy link
Contributor Author

Good idea. I looked at making that change to the output and it isn't working the way I expect it to, so I'll need to investigate how to implement that correctly. Let's go ahead and merge this PR and I'll follow up on the output message enhancement with this ticket: #531

@maxme
Copy link
Contributor

maxme commented Nov 16, 2017

:shipit:

@maxme maxme merged commit 6747aa7 into develop Nov 16, 2017
@maxme maxme deleted the add/high-priority-ui-tests branch November 16, 2017 13:32
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.

4 participants