Closed Bug 898613 Opened 11 years ago Closed 9 years ago

Add back button tests for entering/exiting editing mode

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: mcomella, Assigned: vivek, Mentored)

References

Details

(Whiteboard: [lang=java])

Attachments

(1 file, 5 obsolete files)

Specifically as a followup to the fixes in bug 895828.

This would include clicking the url bar (is it selected?), pressing back to deselect, and pressing back a second time to dismiss editing mode.

This is probably the most appropriate under a file called testEditingMode, or something similar.
Status: NEW → ASSIGNED
I think this is causing some current tests to fail. See:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25796589&tree=Fig

Unfortunately, bug 898887 is making it hard to tell if this is a consistent failure or an intermittent failure.
Experimenting with backing out bug 895828 (and the bugs that looked like they caused bug 898887), to see if that fixes things:

https://tbpl.mozilla.org/?tree=Try&rev=b75622210038
It looks like bug 895828 didn't cause persistent failures (I retriggered the rc2 job from when that landed and it went green), so I won't land a backout of bug 895828 on fig.
FYI: I kinda added tests for this behaviour in bug 896571. We might want to factor it out into a separate test for clarity. Up to you.
IS there anything actionable about this bug?
Flags: needinfo?(michael.l.comella)
Yes or no, depending on how meticulous we're being. We can:
    1) Ensure editing mode behavior (specifically entering and exiting) is consistent for both about:home and for replacing the current tab's URL
    2) Use View.isSelected() to make sure the selection behavior is as expected
    3) More closely test the swipe vs. back button behavior and how it affects entering/exiting editing mode 

Feel free to close if you feel this is too meticulous, but I'm happy to write up the tests (even if it's just so I can better understand Robocop though feel free to tell me other areas which may need more robust testing if you want me to do them instead).
Flags: needinfo?(michael.l.comella)
This is a new test, no need to block bug 895673.
Blocks: new-about-home
No longer blocks: 895673
OS: Linux → Android
Hardware: x86_64 → All
We have been implicitly testing the back button behavior in UITest, however, in bug 997996, we started to use the URL bar's cancel button instead since it's more reliable. We should add some tests to ensure the back button behavior does not break so I'm updating this bug for the purpose.
Assignee: michael.l.comella → nobody
Component: General → Awesomescreen
Depends on: 997996
Summary: [fig] Add tests for editing mode - back button behavior and selection → Add back button tests for entering/exiting editing mode
Whiteboard: abouthome-hackathon → [mentor=mcomella][lang=java]
Component: Awesomescreen → Testing
I spoke with vivek on IRC.
Assignee: nobody → vivekb.balakrishnan
Attached patch 898613.patch (obsolete) — Splinter Review
Attachment #8415572 - Flags: review?(michael.l.comella)
Comment on attachment 8415572 [details] [diff] [review]
898613.patch

Review of attachment 8415572 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, looking pretty good! :)

::: mobile/android/base/tests/components/ToolbarComponent.java
@@ +54,5 @@
> +        fAssertTrue("The edit text is selected", isUrlEditTextSelected());
> +        return this;
> +    }
> +
> +    public ToolbarComponent assertIsNotUrlEditTextSelected() {

nit: -> assertIsUrlEditTextNotSelected

::: mobile/android/base/tests/robocop.ini
@@ +14,5 @@
>  [testAwesomebar]
>  [testAxisLocking]
>  # disabled on x86 only; bug 927476
>  skip-if = processor == "x86"
> +[testBackButtonInEditMode]

nit: This should be below the "# Using UITest" line at the bottom of the file.

For your edification, note that the skip-if's affect the test directly below them. If we were to keep the test here, that means we'd want to move it up, under "testAxisLocking".

::: mobile/android/base/tests/testBackButtonInEditMode.java
@@ +12,5 @@
> +        GeckoHelper.blockForReady();
> +
> +        // Verify back button behavior for edit mode.
> +        mToolbar.enterEditingMode();
> +        mToolbar.assertIsUrlEditTextSelected();

nit:

mToolbar.enterEditingMode()
        .assertIsUrlEditTextSelected();

// Here and below.

@@ +27,5 @@
> +        // Verify the swipe behavior in edit mode.
> +        mToolbar.enterEditingMode();
> +        getSolo().scrollToSide(getSolo().LEFT);
> +        mToolbar.assertIsNotUrlEditTextSelected();
> +        mToolbar.assertIsEditing();;

You have two semicolons here! Be sure to compile and run this! Remember that robocop is built independently of the browser (i.e. use `mach build build/mobile/robocop` to build robocop)

@@ +40,5 @@
> +    }
> +
> +    public void testExitUsingBackButton() {
> +        getSolo().goBack();
> +        mToolbar.assertIsNotUrlEditTextSelected();

I think you should remove this line. Since we're not in editing mode (and test it on the next line), the url EditText should no longer be visible. We want to avoid asserting non-visible state because developers can set those values to whatever they want (e.g. maybe we reselect the url edit text when the toolbar is hidden so Android doesn't have to redraw when it's selected again).
Attachment #8415572 - Flags: review?(michael.l.comella) → review-
Attached patch 898613.patch (obsolete) — Splinter Review
Feedbacks incorporated.

In robocop.ini, the skip-ifs apply to the test list above it. 
Reference : https://bugzilla.mozilla.org/show_bug.cgi?id=977167#c12 here
Attachment #8415572 - Attachment is obsolete: true
Attachment #8415696 - Flags: review?(michael.l.comella)
Comment on attachment 8415696 [details] [diff] [review]
898613.patch

Review of attachment 8415696 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to vivek from comment #12)
> In robocop.ini, the skip-ifs apply to the test list above it. 
> Reference : https://bugzilla.mozilla.org/show_bug.cgi?id=977167#c12 here

So it seems - nice catch!

::: mobile/android/base/tests/testBackButtonInEditMode.java
@@ +41,5 @@
> +
> +    public void testExitUsingBackButton() {
> +        getSolo().goBack();
> +        mToolbar.assertIsNotEditing()
> +                .assertIsUrlEditTextNotSelected();

We want to remove this, right? Do you still have questions about it?
Attachment #8415696 - Flags: review?(michael.l.comella) → review-
Attached patch 898613.patch (obsolete) — Splinter Review
Added check in ToolbarComponent.enterUrl to verify if urledittext is still the isInputMethodTarget. If not urledittext is reselected to have the keyboard displayed. 

Deployed the build in try server and logs are available at 
https://tbpl.mozilla.org/?tree=Try&rev=9df7d0b28d8e
Attachment #8415696 - Attachment is obsolete: true
Attachment #8416458 - Flags: review?(michael.l.comella)
Comment on attachment 8416458 [details] [diff] [review]
898613.patch

Review of attachment 8416458 [details] [diff] [review]:
-----------------------------------------------------------------

It seems your try build failed. :\ See below for some suggestions.

::: mobile/android/base/tests/components/ToolbarComponent.java
@@ +192,5 @@
>  
>          mSolo.clearEditText(urlEditText);
>          mSolo.enterText(urlEditText, url);
>  
> +        // Keyboard input is dismissed in previous call which is counter intuitive.

I would agree that it is a bit counter intuitive, but I believe the url bar should still be selected even when it is not the input method target.

One thing that is helpful when writing tests is to check against the robotium source code to see what's actually going on. When you do this, make sure you're looking at the robotium version that we're running (see Makefile.in in [1])! In this case, 4.3.1. I generally find it easiest to navigate Robotium locally with vim and ctags (I'm not sure what you're dev environment is like).

For this case, you'll notice that `enterText` calls `DialogUtils.hideSoftKeyboard` before it even enters text [2]!

However, in doing this, I noticed there's another Robotium text entry method, namely "typeText". This sounds much more like something a user would do (rather than calling `.setText` on the url bar) and might end in a more intuitive and expected state from when you interact with the browser. Maybe using this method instead will fix the issues you might be seeing? (both work for me locally).

If not, maybe looking at the Robotium source will give you some new insight in how to fix the issue!

[1]: https://mxr.mozilla.org/mozilla-central/search?string=robotium-&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
[2]: https://github.com/RobotiumTech/robotium/blob/robotium-4.3.1/robotium-solo/src/main/java/com/jayway/android/robotium/solo/TextEnterer.java#L54
Attachment #8416458 - Flags: review?(michael.l.comella) → review-
Attached patch 898613.patch (obsolete) — Splinter Review
New patch that uses typeText to simulate text entry in edit mode.
Patch tested in try server.
https://tbpl.mozilla.org/?tree=Try&rev=5ae8bc2262f0
Attachment #8416458 - Attachment is obsolete: true
Attachment #8418924 - Flags: review?(michael.l.comella)
Comment on attachment 8418924 [details] [diff] [review]
898613.patch

Review of attachment 8418924 [details] [diff] [review]:
-----------------------------------------------------------------

Just one nit!

::: mobile/android/base/tests/testBackButtonInEditMode.java
@@ +25,5 @@
> +        testExitUsingBackButton();
> +
> +        // Verify the swipe behavior in edit mode.
> +        mToolbar.enterEditingMode()
> +            .assertIsUrlEditTextSelected();

nit: Indentation.
Attachment #8418924 - Flags: review?(michael.l.comella) → review-
Attached patch 898613.patch (obsolete) — Splinter Review
nit corrected.
Attachment #8418924 - Attachment is obsolete: true
Attachment #8419025 - Flags: review?(michael.l.comella)
Comment on attachment 8419025 [details] [diff] [review]
898613.patch

Review of attachment 8419025 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm! Thanks!
Attachment #8419025 - Flags: review?(michael.l.comella) → review+
Keywords: checkin-needed
(In reply to vivek from comment #16)
> New patch that uses typeText to simulate text entry in edit mode.
> Patch tested in try server.
> https://tbpl.mozilla.org/?tree=Try&rev=5ae8bc2262f0

Were the testInputConnection failures in this Try run resolved?
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> Were the testInputConnection failures in this Try run resolved?

I wonder if the `typeText` change in ToolbarComponent could be to blame here.

Vivek, do you mind taking a look?
Flags: needinfo?(vivekb.balakrishnan)
I tested a patch in try server without the 'typeText' change in ToolbarComponent. testInputConnection didn't fail in that patch. 

https://tbpl.mozilla.org/?tree=Try&rev=4467dee1333f

I'll dig into the testInputConnection to ascertain why the patch with 'typeText' changes was failing.
Flags: needinfo?(vivekb.balakrishnan)
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java] → [lang=java]
Still working on this one, Vivek?
Flags: needinfo?(vivekb.balakrishnan)
yes, I would like to see this issue to the end.
Flags: needinfo?(vivekb.balakrishnan)
Attached patch 898613.patchSplinter Review
A new patch rebased with latest tree. 

Try run log:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ef09a69cf6b3
Attachment #8532301 - Flags: checkin?
https://hg.mozilla.org/integration/fx-team/rev/fe2dcc34d83d
Whiteboard: [lang=java] → [lang=java][fixed-in-fx-team]
Attachment #8419025 - Attachment is obsolete: true
Attachment #8532301 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/fe2dcc34d83d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [lang=java][fixed-in-fx-team] → [lang=java]
Target Milestone: --- → Firefox 37
Depends on: 1113751
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.