Closed Bug 980074 Opened 10 years ago Closed 10 years ago

Tests for text selection

Categories

(Firefox for Android Graveyard :: Text Selection, defect)

Product:
Firefox for Android Graveyard
Component:
Text Selection
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 32

People

(Reporter: wesj, Assigned: capella)

References

Details

Attachments

(3 files, 4 obsolete files)

bug980074p0.diff
16.78 KB, patch
wesj
: review+
Details | Diff | Splinter Review
WIP Tests for Action modes
19.71 KB, patch
Details | Diff | Splinter Review
bug980074p1.diff Tweak to disable test on Android 2.3
1.37 KB, patch
wesj
: review+
Details | Diff | Splinter Review 
We keep breaking text selection. We should write some tests for it. I'll try to start out basic and we can build this out as we grow.

As a later, harder part, some of our issues have come from different keyboards. It would be nice if we could install and test a few different ones.
Attached patch Patch 1/? (obsolete) — — Splinter Review 
This is a start... I made this into basically a unit test for SelectionHandler, although it doesn't test much there yet even. If just uses SelectionHandler.startSelection to select a div and an input and checks that selection and state sorta look ok.

I don't think JSTest quite had what I needed to do this (i.e. the ability to load a page and inspect the DOM in it), so this uses roboextender and Promises pretty extensively. I like it :)

I'm curious if you guys have any ideas/feedback here, but I think maybe something simpler like this is the first step to take. We can expand things out to check the actionbar is in the correct state, make sure handles are showing, etc in later patches...
Attachment #8386586 - Flags: feedback?(markcapella)
Attachment #8386586 - Flags: feedback?(margaret.leibovic)
Component: General → Text Selection
OS: Linux → Android
Hardware: x86_64 → ARM
Comment on attachment 8386586 [details] [diff] [review]
Patch 1/?

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

This looks like a good start, I'm just a bit suspicious about the promises logic.

::: mobile/android/base/tests/roboextender/textSelection.html
@@ +2,5 @@
> +  <head>
> +    <title>IconGrid test page</title>
> +    <meta name="viewport" content="initial-scale=1.0"/>
> +    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
> +    <script type="application/javascript">

Can we include script files here? I feel like this JS is getting big enough that it would be nice in its own file.

@@ +20,5 @@
> +  var div = document.getElementById("divOne");
> +  var div2 = document.getElementById("divTwo");
> +
> +  // Test that calling startSelection with a node selects all the text in the oage
> +  var sh = win.SelectionHandler;

If you're only using the chrome window to get SelectionHandler, maybe we should just have a lazy getter for SelectionHandler that does that. Mainly to avoid confusion over this `win` variable as this test grows.

@@ +24,5 @@
> +  var sh = win.SelectionHandler;
> +  var selection;
> +
> +  // Check the initial state of the selection handler
> +  Promise.all([

How is this working, isn't this supposed to be an array of promises? Do these is/ok assertions return promises? You're really stretching my promise skills here :)

@@ +25,5 @@
> +  var selection;
> +
> +  // Check the initial state of the selection handler
> +  Promise.all([
> +    is(sh._activeType, sh.TYPE_NONE, "Active type is TYPE_NONE"),

Trying to decide how I feel about digging into the SelectionHandler implementation this much... I suppose this is appropriate if this is supposed to be a unit-style test.

@@ +40,5 @@
> +      is(selection.focusNode, document.body.lastChild, "Focus node is correct"),
> +      is(selection.focusOffset, document.body.lastChild.textContent.length, "Focus offset is right"),
> +      is(sh._activeType, sh.TYPE_SELECTION, "Active type is TYPE_SELECTION")
> +    ]);
> +  }).then(function() {

If the promise fails to resolve, will the test fail? If not, we should include error callbacks in these then calls, and fail an assertion if we run into some error.

@@ +47,5 @@
> +      mode: sh.SELECT_AT_POINT,
> +      x: rect.left + rect.width/2,
> +      y: rect.top + rect.height/2
> +    });
> +    var selection = sh._getSelection();

You shouldn't be using var again here, right? I think you should rename this selection variable gSelection or something, to make it clear that you're using a global variable through these tests.

@@ +55,5 @@
> +      is(sh._activeType, sh.TYPE_SELECTION, "Active type is TYPE_SELECTION"),
> +      is(selection.anchorNode, div.firstChild, "Anchor Node is correct"),
> +      ok(selection.anchorOffset > 0, "Anchor offset is > 0"),
> +      is(selection.focusNode, div.firstChild, "Focus node is correct"),
> +      ok(selection.focusOffset < div.textContent.length, "Focus offset is < 0"),

This message is wrong.

@@ +67,5 @@
> +      is(sh._activeType, sh.TYPE_NONE, "Active type is TYPE_NONE"),
> +    ]);
> +  }).then(function() {
> +    testInput();
> +  });

You could just say .then(testInput);

Instead of calling the next test inside the first test, you could just use promises to chain them together in the main start function. You would just need to make sure the test returned the promise.

::: mobile/android/base/tests/testTextSelectionHandler.java
@@ +23,5 @@
> +    public void testTextSelectionHandler() {
> +        blockForGeckoReady();
> +
> +        Actions.EventExpecter expector = mActions.expectGeckoEvent("Robocop:Test");
> +        inputAndLoadUrl("chrome://roboextender/content/textSelection.html");

We should probably use UITest for new tests, since I think the NavigationHelper.enterAndLoadUrl is simpler.

testHomeBanner is an example of a roboextender UITest.

@@ +43,5 @@
> +        }
> +
> +        if (eventData.has("result")) {
> +            boolean res = eventData.optBoolean("result");
> +            mAsserter.ok(eventData.optBoolean("result"), "JS Test", eventData.optString("msg"));

This is good, having the JS specify the message.
Attachment #8386586 - Flags: feedback?(margaret.leibovic) → feedback+
(In reply to :Margaret Leibovic from comment #2)
> How is this working, isn't this supposed to be an array of promises? Do
> these is/ok assertions return promises? You're really stretching my promise
> skills here :)

Heh. Is and ok are defined in this same file. They return promises.

> Trying to decide how I feel about digging into the SelectionHandler
> implementation this much... I suppose this is appropriate if this is
> supposed to be a unit-style test.

This was easy to fix. We have a method that queries just this :). Just trying to find simple things I could test. TBH, I"m not that used to writing these unit tests. SelectionHandler's "public" API looks like:

// TESTED HERE
canSelect
startSelection
selectAll
isSelectionActive

// NOT TESTED DIRECTLY HERE
attachCaret

// INTERFACES, We could fire some fake data into these and see what happens
observe
handleEvent
notifySelectionChanged
subdocumentScrolled

// ACTION API, should probably be in its own object, but tests would be nice either way...
addAction
removeAction
actions <--- This should probably be private
copySelection
shareSelection
searchSelection
Comment on attachment 8386586 [details] [diff] [review]
Patch 1/?

Moar!

I just wrote my first Selection test for a Metro bug I completed and was wishing Fennec had a framework ... this will help !
Attachment #8386586 - Flags: feedback?(markcapella) → feedback+
Do you see pushing this anytime soon? I have a test addition I hacked onto it for InputMethod-specific testing, starting with bug 982608.

To work, I'd have to figure out how to get a subset of test devices equipped with the SwiftKey Inputmethod into the pool ...

That's something I'd like to do anyhow as that's been a source of serious issues lately ... would be helpful to the jchen / IME types too I'd imagine :-)
Attached patch Patch (obsolete) — — Splinter Review 
Cleaned up. I added some basic tests that the selection is closed in certain circumstances (tapping outside, switching tabs, etc). The other events (dragging handles or any of the page actions stuff) seemed easier to test in a different patch.
Attachment #8386586 - Attachment is obsolete: true
Attachment #8392373 - Flags: review?(margaret.leibovic)
Making sure jchen sees this bug.
Comment on attachment 8392373 [details] [diff] [review]
Patch

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

I'm confused by some of your variable declarations, but the direction of this looks good, so I'm fine with us landing it and iterating on it in the tree (as long as there are no intermittent issues).

Also, jchen may be coming up with a better solution for Java/JS communication in bug 983811.

::: mobile/android/base/tests/roboextender/textSelection.html
@@ +1,3 @@
> +<html>
> +  <head>
> +    <title>IconGrid test page</title>

This looks incorrect, but you can probably just get rid of the title.

@@ +17,5 @@
> +    }
> +
> +    return selectionHandler;
> +  }
> +})();

Why do you need this immediately-invoked function expression? Just to encapsulate the selectionHandler variable? I feel like using an XPMCOMUtils lazy getter would be easier to read.

@@ +32,5 @@
> +    });
> +}
> +
> +function testSelectAllDivs() {
> +  var sh = getSelectionHandler();

Why do you always like using var? :)

@@ +34,5 @@
> +
> +function testSelectAllDivs() {
> +  var sh = getSelectionHandler();
> +  var div = document.getElementById("divOne");
> +  var div2 = document.getElementById("divTwo");

Nit: I would call this divOne/divTwo to match their ids (or at least div1 instead of just div)

@@ +37,5 @@
> +  var div = document.getElementById("divOne");
> +  var div2 = document.getElementById("divTwo");
> +
> +  // Test that calling startSelection with a node selects all the text in the oage
> +  var selection;

Why is this variable declared here?

@@ +69,5 @@
> +    mode: sh.SELECT_AT_POINT,
> +    x: rect.left + rect.width/2,
> +    y: rect.top + rect.height/2
> +  });
> +  selection = sh._getSelection();

You're missing a var here.

@@ +120,5 @@
> +
> +function testSelectPointInput() {
> +  var sh = getSelectionHandler();
> +
> +  var selection;

Just declare this variable where you assign in down below.

@@ +210,5 @@
> +      result: one === two,
> +      msg: msg + " : " + one + " === " + two
> +    }, function(res, err) {
> +      if (err) reject(err);
> +      else resolve(res)

Braces and semi-colons, please!

::: mobile/android/base/tests/testTextSelectionHandler.java
@@ +21,5 @@
> +
> +    public void testTextSelectionHandler() {
> +        GeckoHelper.blockForReady();
> +
> +        Actions.EventExpecter expector = getActions().expectGeckoEvent("Robocop:Test");

Nit: expecter (to match class name)

@@ +31,5 @@
> +
> +        expector.unregisterListener();
> +    }
> +
> +    private boolean test(Actions.EventExpecter expect) {

Nit: expecter.

Also, add some documentation explaining what this method is doing.

@@ +41,5 @@
> +            return false;
> +        }
> +
> +        if (eventData.has("result")) {
> +            boolean res = eventData.optBoolean("result");

You're not using this variable.
Attachment #8392373 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8392373 [details] [diff] [review]
Patch

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

Looks good! Bug 983811 will simplify a lot of the communication between JS and Java, but that's not ready yet, so you should go ahead with this patch. Some drive-by comments:

::: mobile/android/base/tests/roboextender/textSelection.html
@@ +1,3 @@
> +<html>
> +  <head>
> +    <title>IconGrid test page</title>

In my experience having a title is good for testing the page was actually loaded, by asserting the title on the Java side after loading the page.

::: mobile/android/base/tests/testTextSelectionHandler.java
@@ +22,5 @@
> +    public void testTextSelectionHandler() {
> +        GeckoHelper.blockForReady();
> +
> +        Actions.EventExpecter expector = getActions().expectGeckoEvent("Robocop:Test");
> +        NavigationHelper.enterAndLoadUrl("chrome://roboextender/content/textSelection.html");

You would add mToolbar.assertTitle here. For examples, see http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testSessionHistory.java#15
Assignee: nobody → markcapella
status posting ... (fyi to me)
Attached patch bug980074p0.diff (obsolete) — — Splinter Review 
Take a look at this version ... previous comment nits addressed and some minor changes ... |landing it and iterating| is a great idea ... also I added a test for a patch I'm about to land (bug 895463) ... it demonstrates that the issue exists, and when the patch lands we'll know, as this test will start failing in TRY and I'll fix it :-D
Attachment #8392373 - Attachment is obsolete: true
Attachment #8407757 - Attachment is obsolete: true
Attachment #8408896 - Flags: review?(wjohnston)
Attached patch bug980074p0.diff — — Splinter Review 
I'm re-posting this without the test for bug 895463 I had added. Repeated TRY tests showed it generating random oranges, so I decided to just decouple the test and tinker with it offline in the context of the actual bug it's associated with.

new TRY push: https://tbpl.mozilla.org/?tree=Try&rev=d9f69e8228f2
Attachment #8408896 - Attachment is obsolete: true
Attachment #8408896 - Flags: review?(wjohnston)
Attachment #8410635 - Flags: review?(wjohnston)
Comment on attachment 8410635 [details] [diff] [review]
bug980074p0.diff

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

Its a start! Thanks :)
Attachment #8410635 - Flags: review?(wjohnston) → review+
Thanks!

Getting ready for the next test addition for bug 895463 and working through the random oranges I encountered originally, has pointed out some subtleties ... mostly involving timing of gecko events initiated in one test surviving past a selection close/start and poisoning the next set of tests.

More info later :)
No idea if this will be useful, but this was my WIP for a second set here to test Action mode stuff. That turned out to be a bit racy, so there's some subtle code in here to work around that. Might be useful ?
https://hg.mozilla.org/mozilla-central/rev/ff8ebfcf68ff
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Comment on attachment 8410635 [details] [diff] [review]
bug980074p0.diff

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

Drive-by, since this was backed out.

::: mobile/android/base/tests/robocop.ini
@@ +80,5 @@
>  skip-if = android_version == "10"
>  [testPrefsObserver]
>  [testPrivateBrowsing]
>  [testPromptGridInput]
> +[testSelectionHandler]

This should be at the bottom of the file - i.e. under "# Using UITest"

::: mobile/android/base/tests/testSelectionHandler.java
@@ +9,5 @@
> +
> +import org.json.JSONObject;
> +
> +
> +public class testSelectionHandler extends UITest {

Why not JavascriptTest? (https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/JavascriptTest.java?rev=96eefe207020#14)
JavascriptTest wasn't really designed for this. i.e. this needs to load a page and interact with it. JavascriptTest is for more xpcshell like things.
Trying to get back to this ... (not sure what happened offhand) ... I'd run several trials of 400 back-to-back on my local machine without any issues on N7 and GS3 before sending to TRY for another 20-ish full tests ... so 

"confused"

https://tbpl.mozilla.org/?tree=Try&rev=d9f69e8228f2
Keep in mind that this is only an Android 2.3 failure (and may even be a bug in the Android framework). I wonder if we can't land with 2.3 disabled so we can at least have as much coverage as this grants us.
Is this the correct syntax? I hadn't seen this before so I just cut'n paste-d it, but on TRY the test still seem to execute ... 

https://hg.mozilla.org/try/rev/2969cd1eff16
https://tbpl.mozilla.org/?tree=Try&rev=b9434433de8c
(In reply to Mark Capella [:capella] from comment #27)
> Is this the correct syntax? I hadn't seen this before so I just cut'n
> paste-d it, but on TRY the test still seem to execute ... 

It seems skip-if actually goes below the test you want to skip. See bug 977167 comment 12.
Ah right... -1 karma point for not simply reading manifestparser.py, +1 karma point for actually catching it on TRY ...

w/Good patch on TRY: https://tbpl.mozilla.org/?tree=Try&rev=e2f136ae9ee5
Mochi-Remote INFO | TEST-INFO | skipping testSelectionHandler | skip-if: android_version == "10"
Attachment #8416289 - Flags: review?(wjohnston)
Attachment #8416289 - Flags: review?(wjohnston) → review+
https://hg.mozilla.org/mozilla-central/rev/76f2454c7866
https://hg.mozilla.org/mozilla-central/rev/0a11d0e5a709
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: