Closed Bug 834370 Opened 12 years ago Closed 12 years ago

Work - Add Content Selection Browser Chrome Tests

Categories

(Firefox for Metro Graveyard :: Tests, defect, P5)

Product:
Firefox for Metro Graveyard
Component:
Tests
Platform:
x86_64
Windows 8.1
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 23

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [selection])

Attachments

(6 files, 15 obsolete files)

part 1 - remove old state tracking
(deleted), patch
sfoster
: review+
Details | Diff | Splinter Review
text area selection tests v.1
(deleted), patch
rsilveira
: review+
Details | Diff | Splinter Review
sub frame content tests v.1
(deleted), patch
rsilveira
: review+
Details | Diff | Splinter Review
part 2 - head helpers v.2
(deleted), patch
mbrubeck
: review+
Details | Diff | Splinter Review
part 3 - selection test helpers
(deleted), patch
mbrubeck
: review+
Details | Diff | Splinter Review
part 4 - basic selection tests v.1
(deleted), patch
sfoster
: review+
Details | Diff | Splinter Review
No description provided.
Summary: Add content selection browser chrome tests → Work - Add content selection browser chrome tests
Whiteboard: feature=work
Blocks: 831985
Attached patch patch v.1 (obsolete) (deleted) — — Splinter Review
Attachment #718987 - Flags: review?(sfoster)
Comment on attachment 718987 [details] [diff] [review] patch v.1 Review of attachment 718987 [details] [diff] [review]: ----------------------------------------------------------------- I got some test failures when I ran this: mochitest-metro-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | copy text test - Got the world, expected The rabbit TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | selection test - Got nothing so VER, expected nothing so VERY TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | selection test - Got nothing so VERY remarkable in that; nor did Alice think it so, expected nothing so VERY remarkable in that; nor did Alice think it so TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | selection test - Got down, expected moment TEST-UNEXPECTED-FAIL | chrome://mochitests/content/metro/browser_selection_tests.js | Test timed out ... I saw your comments on bugs found, are these expected failures? See notes on https://bugzilla.mozilla.org/show_bug.cgi?id=846685 and the proposed setUp, run, tearDown fixture methods. ::: browser/metro/base/tests/browser_selection_tests_02.html @@ +6,5 @@ > + > +<body> > + > +<form action="texarea.html" method="submit" autocomplete="on"> > +<center> Nit: Center? Really? :) To avoid moments of mild nausea in our fellow front-enders lets use margin: 0 auto; on the textarea and the other centered-things in these tests ::: browser/metro/base/tests/head.js @@ +580,5 @@ > info(gCurrentTest.desc); > yield Task.spawn(gCurrentTest.run); > info("END "+gCurrentTest.desc); > + if (gCurrentTest.cleanup) > + gCurrentTest.cleanup(); See https://bugzilla.mozilla.org/show_bug.cgi?id=846685, I'll attach a patch there which extends our test lifecycle to support optional, optionally async setUp and tearDown method. If you don't mind making the same change in your tests we'll avoid some conflicts when I land my stuff :)
Attachment #718987 - Flags: review?(sfoster) → review-
For the test failures, were you able to get those reliably over a few test runs? I'm curious if these are in random orange territory or if there's something different between our machines that cause this.
I ran the tests 3 times and they seemed to fail in the same place each time.
Attached patch test helpers v.1 (obsolete) (deleted) — — Splinter Review
Lots of little tweaks to try and get these running more reliably.
Attachment #718987 - Attachment is obsolete: true
Attachment #720440 - Flags: review?(sfoster)
Attached patch tests v.1 (obsolete) (deleted) — — Splinter Review
Attachment #720442 - Flags: review?(sfoster)
Attached patch html (obsolete) (deleted) — — Splinter Review
Attached patch updated html (obsolete) (deleted) — — Splinter Review
- updated per comments
Attachment #720443 - Attachment is obsolete: true
Attached patch check for landscape mode (obsolete) (deleted) — — Splinter Review
Chatted with mbrubeck about this on irc. I think your test failures are due to running the tests in portrait mode. If not please let me know. Since these tests require a wide screen, we're going to opt out of them if someone runs them in a mode other than landscape.
Attachment #720794 - Flags: review?(sfoster)
Comment on attachment 720794 [details] [diff] [review] check for landscape mode >+ info("browser_selection_tests need landscape mode to run."); Could you change this from "info(...)" to "todo(false, ...)"? Just so it'll be a little more visible in the summary when the tests complete.
Comment on attachment 720440 [details] [diff] [review] test helpers v.1 Review of attachment 720440 [details] [diff] [review]: ----------------------------------------------------------------- Just some nits and thoughts below, looks good otherwise. ::: browser/metro/base/tests/head.js @@ +102,5 @@ > > +function showNavBar() > +{ > + let promise = waitForEvent(Elements.tray, "transitionend"); > + if (!ContextUI.isVisible) { It seems odd to create a promise but only resolve it in the if clause. This triggers a warning on sometimes having no return value. However, provided showNavBar is called via a Task.spawn it should handle either eventuality @@ +110,5 @@ > +} > + > +function fireAppBarDisplayEvent() > +{ > + let promise = waitForEvent(Elements.tray, "transitionend"); This makes me think that the appbar should provide its own shown/hidden events rather than making us hook into transitionend (and it you look at ContextUI you'll see there's a further setTimeout after that). But, works for now @@ +554,5 @@ > + EventUtils.synthesizeTouchAtPoint(this._endPoint.xPos, this._endPoint.yPos, > + { type: "touchend" }, this._win); > + purgeEventQueue(); > + this._win = null; > + }, nit: trailing comma. @@ +598,5 @@ > } > + try { > + yield Task.spawn(gCurrentTest.run.bind(gCurrentTest)); > + } catch (ex) { > + ok(false, "gCurrentTest.run", ex.message); All tests should have a .desc property, so we can improve the error message here by including that in the output
Attachment #720440 - Flags: review?(sfoster) → review+
Comment on attachment 720794 [details] [diff] [review] check for landscape mode Review of attachment 720794 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #720794 - Flags: review?(sfoster) → review+
We were wondering yesterday if you were running these in landscape mode. Was that the case? I did so and ended up with failures similar to to the log you posted to pastebin yesterday.
Component: Input → Tests
I think I found a flaw in this that when fixed smooths things out. We don't treat touchup like a touchmove when calculating selection in the page. So in the tests, if the difference between TouchDragAndHold's last touchmove and touchend is wide enough such that we don't snap to the right word, tests might fail. DOM's snap seems a bit unpredictable in general. I'll try to work up a test case for this a file a bug on selection. In the mean time though I'm testing some fixes that should improve the reliability of these tests.
on my surface pro, which was failing pretty bad: Browser Chrome Test Summary Passed: 81 Failed: 0 Todo: 0 woot!
Attachment #720442 - Attachment is obsolete: true
Attachment #720442 - Flags: review?(sfoster)
Comment on attachment 720794 [details] [diff] [review] check for landscape mode Could you make the browser_context_menu_tests.js bail out on non-landscape screens too?
These failures all seem to stem from latency in message manager. The tests rely on selection manager values (selection state, monocle xPos) while walking through steps in each test. I've tried compensating for this using purgeEventQueue calls, but this doesn't guarantee all mm events are delivered since one mm message can trigger additional message that a call to purgeEventQueue won't pick up on. I'll try to add additional state getters to the selection code to help compensate for this.
(In reply to Matt Brubeck (:mbrubeck) from comment #16) > Comment on attachment 720794 [details] [diff] [review] > check for landscape mode > > Could you make the browser_context_menu_tests.js bail out on non-landscape > screens too? sure, will do.
Attached patch changes to selection handlers (obsolete) (deleted) — — Splinter Review
This adds better SelectionHandler state tracking to SelectionHelperUI.
Attachment #721716 - Flags: review?(mbrubeck)
Attached patch tests v.2 (obsolete) (deleted) — — Splinter Review
Attachment #721717 - Flags: review?(sfoster)
Attached patch rollup (obsolete) (deleted) — — Splinter Review
Attached patch rollup (obsolete) (deleted) — — Splinter Review
rollup with misc. nits from reviews addressed. I also removed some old commenting in the selection helpers.
Attachment #721720 - Attachment is obsolete: true
Comment on attachment 721716 [details] [diff] [review] changes to selection handlers Review of attachment 721716 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/metro/base/content/contenthandlers/SelectionHandler.js @@ +51,5 @@ > _selectionMoveActive: false, > _lastMarker: "", > _debugOptions: { dumpRanges: false, displayRanges: false }, > _snap: true, > + _steadyState: [], Can we use a counter instead of an array here? Basically this is nonzero if there is an operation in progress, and zero otherwise, right? Could you add a brief comment explaining that?
Attachment #721716 - Flags: review?(mbrubeck) → review+
Attached patch rollup v.2 (obsolete) (deleted) — — Splinter Review
Changes include: - listening for scroll completion on page and frame scrolls - upping various timeouts and requesting more time for the overall test via requestLongerTimeout (for debug builds) - decreasing the number of touch events sent in TouchDragAndHold to cut down on latency - added "browser normalize time" header tests for each page load
Attachment #721725 - Attachment is obsolete: true
also - - added start screen visible check on first page load
Comment on attachment 722409 [details] [diff] [review] rollup v.2 Review of attachment 722409 [details] [diff] [review]: ----------------------------------------------------------------- See inline comments, and test failures at: http://pastebin.mozilla.org/2201668 ::: browser/metro/base/tests/browser_selection_tests.js @@ +169,5 @@ > + > + // check for active selection > + is(getTrimmedSelection(gWindow).toString(), "", "selection test"); > + }, > + tearDown: function tearDown() { Would it be worth calling addTab in the setUp for each test and starting with a clean sheet? (and kill the tab in tearDown) @@ +558,5 @@ > + return SelectionHelperUI.isHandlerSteady; > + }, kCommonWaitMs, kCommonPollMs); > + > + // active state > + is(SelectionHelperUI.isActive, true, "selection active"); ok() not is() @@ +658,5 @@ > + yield hideContextUI(); > + > + gWindow = Browser.selectedTab.browser.contentWindow; > + > + yield waitForMs(3000); what are we waiting for here? addTab waits for pageshow - is that too early maybe? ::: browser/metro/base/tests/head.js @@ +106,5 @@ > } > > +function showNavBar() > +{ > + let promise = waitForEvent(Elements.tray, "transitionend"); Note that ContextUI has another setTimeout in its transitionend handler, which calls ContentAreaObserver.updateContentArea, not sure if that's useful or relevant? I think the promise should be returned in either case, just setTimeout 0 and resolve it if isVisible is true @@ +115,5 @@ > +} > + > +function fireAppBarDisplayEvent() > +{ > + let promise = waitForEvent(Elements.tray, "transitionend"); We were kicking around an idea for ContextUI.dismiss to return a promise that resolves when it is truly done dimissing and transitioning, see #849015
Attachment #722409 - Flags: review-
(In reply to Sam Foster [:sfoster] from comment #26) > Comment on attachment 722409 [details] [diff] [review] > rollup v.2 > > Review of attachment 722409 [details] [diff] [review]: > ----------------------------------------------------------------- > > See inline comments, and test failures at: > http://pastebin.mozilla.org/2201668 > > ::: browser/metro/base/tests/browser_selection_tests.js > @@ +169,5 @@ > > + > > + // check for active selection > > + is(getTrimmedSelection(gWindow).toString(), "", "selection test"); > > + }, > > + tearDown: function tearDown() { > > Would it be worth calling addTab in the setUp for each test and starting > with a clean sheet? (and kill the tab in tearDown) I think that would make things even worse. Opening tabs brings up the context ui and in debug builds, can take a very very long time to complete. The browser also needs to stabilize afterward. > > @@ +558,5 @@ > > + return SelectionHelperUI.isHandlerSteady; > > + }, kCommonWaitMs, kCommonPollMs); > > + > > + // active state > > + is(SelectionHelperUI.isActive, true, "selection active"); > > ok() not is() > > @@ +658,5 @@ > > + yield hideContextUI(); > > + > > + gWindow = Browser.selectedTab.browser.contentWindow; > > + > > + yield waitForMs(3000); > > what are we waiting for here? addTab waits for pageshow - is that too early > maybe? open tab overhead. Really not needed on release builds, but debug builds need a lot of time to "calm down" before the test runs. > > ::: browser/metro/base/tests/head.js > @@ +106,5 @@ > > } > > > > +function showNavBar() > > +{ > > + let promise = waitForEvent(Elements.tray, "transitionend"); > > Note that ContextUI has another setTimeout in its transitionend handler, > which calls ContentAreaObserver.updateContentArea, not sure if that's useful > or relevant? > I think the promise should be returned in either case, just setTimeout 0 and > resolve it if isVisible is true > > @@ +115,5 @@ > > +} > > + > > +function fireAppBarDisplayEvent() > > +{ > > + let promise = waitForEvent(Elements.tray, "transitionend"); > > We were kicking around an idea for ContextUI.dismiss to return a promise > that resolves when it is truly done dimissing and transitioning, see #849015 sounds good.
Depends on: 849015
Maybe after we get tests running in automation and have the time to work on test stability we can revisit this. But currently there's no way tests this complex can run reliably on mc.
Assignee: jmathies → nobody
Attachment #721717 - Flags: review?(sfoster)
Blocks: metrov1it4
Priority: -- → P1
Summary: Work - Add content selection browser chrome tests → Change - Add Content Selection Browser Chrome Tests
Whiteboard: feature=work → feature=change c=Content_features u=metro_firefox_user p=0
Blocks: metrov1triage
No longer blocks: metrov1it4
Priority: P1 → P5
Blocks: caret-sel
Whiteboard: feature=change c=Content_features u=metro_firefox_user p=0 → feature=change c=Content_features u=metro_firefox_user p=0 [selection]
Blocks: sel-tests
No longer blocks: caret-sel
No longer blocks: metrov1triage
Summary: Change - Add Content Selection Browser Chrome Tests → Work - Add Content Selection Browser Chrome Tests
Whiteboard: feature=change c=Content_features u=metro_firefox_user p=0 [selection]
Attachment #720444 - Attachment is obsolete: true
Attachment #721716 - Attachment is obsolete: true
Attachment #721717 - Attachment is obsolete: true
Attachment #722409 - Attachment is obsolete: true
Attachment #720794 - Attachment is obsolete: true
Attachment #720440 - Attachment is obsolete: true
Depends on: 854203
Whiteboard: [selection]
Remove some old test related state tracking I'm replacing with better helper routines.
Assignee: nobody → jmathies
Attachment #738632 - Flags: review?(sfoster)
Attached patch head helpers (obsolete) (deleted) — — Splinter Review
Matt, you already approved most of this a while back. I've added one helper - isDebugBuild(), which I'm using to disable selection tests for debug build test runs.
Attachment #738634 - Flags: review?(mbrubeck)
Attached patch selection test helpers (obsolete) (deleted) — — Splinter Review
This adds some props on SelectionHelperUI for better tracking of state, and a neat ping/pong utility call that can be used to insure all message manager messages have been flushed.
Attachment #738636 - Flags: review?(mbrubeck)
Attached patch basic selection tests v.1 (obsolete) (deleted) — — Splinter Review
Basic content selection tests
Attachment #738637 - Flags: review?(sfoster)
Attached patch text area selection tests v.1 (deleted) — — Splinter Review
Attachment #738639 - Flags: review?(rsilveira)
Attached patch sub frame content tests v.1 (deleted) — — Splinter Review
Attachment #738640 - Flags: review?(rsilveira)
I have more of these in the works, but I'd like to get the basics landed first.
Comment on attachment 738632 [details] [diff] [review] part 1 - remove old state tracking Review of attachment 738632 [details] [diff] [review]: ----------------------------------------------------------------- Looks straightforward.
Attachment #738632 - Flags: review?(sfoster) → review+
Comment on attachment 738634 [details] [diff] [review] head helpers For simplicity, I'd prefer to just disable these tests at build time by skipping them in Makefile.in: ifndef MOZ_DEBUG BROWSER_TESTS += ... endif
Attachment #738634 - Flags: review?(mbrubeck) → review-
Attached patch part 2 - head helpers v.2 (deleted) — — Splinter Review
I think I can carry over the original r+ then from the previous review. Feel free to look this over if you like.
Attachment #738634 - Attachment is obsolete: true
(In reply to Jim Mathies [:jimm] from comment #38) > Created attachment 738682 [details] [diff] [review] > head helpers v.2 > > I think I can carry over the original r+ then from the previous review. Feel > free to look this over if you like. My bad, it was from sfoster: review+.
Attachment #738682 - Attachment description: head helpers v.2 → head helpers v.2 [sfoster:r+]
Comment on attachment 738639 [details] [diff] [review] text area selection tests v.1 Review of attachment 738639 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/metro/base/tests/mochitest/browser_selection_textarea.html @@ +3,5 @@ > + <head> > + </head> > +<body> > +<form action="texarea.html" method="submit" autocomplete="on"> > +<div style="text-align: center;"> Please add a fixed margin instead of centering. Centering can cause different monitor sizes to react to sendContextMenu* differently. ::: browser/metro/base/tests/mochitest/browser_selection_textarea.js @@ +55,5 @@ > + > + let promise = waitForEvent(document, "popupshown"); > + sendContextMenuClick(355, 50); > + yield promise; > + ok(promise && !(promise instanceof Error), "promise error"); nit: This is not needed, the yield will throw and this will never be executed in case of an error. Same below. @@ +85,5 @@ > + setUp: setUpAndTearDown, > + tearDown: setUpAndTearDown, > + run: function test() { > + // work around for buggy context menu display > + yield waitForMs(100); Is there a condition we could use? @@ +127,5 @@ > + textLength = newTextLength; > + return false; > + } > + return true; > + }, 45000, 1000); Wow, that's a huge timeout! :) Can we reduce it?
Attachment #738639 - Flags: review?(rsilveira) → review+
Comment on attachment 738640 [details] [diff] [review] sub frame content tests v.1 Review of attachment 738640 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/metro/base/tests/mochitest/browser_selection_frame_content.js @@ +121,5 @@ > + > + let scrollPromise = waitForEvent(gFrame.contentDocument.defaultView, "scroll"); > + gFrame.contentDocument.defaultView.scrollBy(0, 200); > + yield scrollPromise; > + ok(scrollPromise && !(scrollPromise instanceof Error), "scrollPromise error"); Nit: not needed, yield will throw when there is an error. Same bellow. @@ +148,5 @@ > + yield popupPromise; > + ok(popupPromise && !(popupPromise instanceof Error), "promise error"); > + > + // copy text routes through message manager > + yield waitForMs(500); You can use a waitForCondition like in https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/browser_context_menu_tests.js?rev=37fd7284be3a#77
Attachment #738640 - Flags: review?(rsilveira) → review+
Attachment #738682 - Flags: review+
No longer depends on: 849015
Attachment #738632 - Attachment description: remove old state tracking → part 1 - remove old state tracking
Attachment #738682 - Attachment description: head helpers v.2 [sfoster:r+] → part 2 - head helpers v.2
Attached patch part 3 - selection test helpers (deleted) — — Splinter Review
Attachment #738636 - Attachment is obsolete: true
Attachment #738636 - Flags: review?(mbrubeck)
Attachment #739135 - Flags: review?(mbrubeck)
Attached patch part 4 - basic selection tests v.1 (obsolete) (deleted) — — Splinter Review
Attachment #738637 - Attachment is obsolete: true
Attachment #738637 - Flags: review?(sfoster)
Attachment #739137 - Flags: review?(sfoster)
Attachment #739135 - Flags: review?(mbrubeck) → review+
Removing the double inclusions in the Makefile that snuck in.
Attachment #739137 - Attachment is obsolete: true
Attachment #739137 - Flags: review?(sfoster)
Attachment #739513 - Flags: review?(sfoster)
Comment on attachment 739513 [details] [diff] [review] part 4 - basic selection tests v.1 Review of attachment 739513 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. All pass for me (Sony Vaio Duo 11)
Attachment #739513 - Flags: review?(sfoster) → review+
(In reply to Rodrigo Silveira [:rsilveira] from comment #40) > Please add a fixed margin instead of centering. Centering can cause > different monitor sizes to react to sendContextMenu* differently. > > nit: This is not needed, the yield will throw and this will never be > executed in case of an error. Same below. updated. > @@ +85,5 @@ > > + setUp: setUpAndTearDown, > > + tearDown: setUpAndTearDown, > > + run: function test() { > > + // work around for buggy context menu display > > + yield waitForMs(100); > > Is there a condition we could use? Nothing reliable. What we need to do is fix the context menu code such that showing context menus in close succession works reliably. I'm guessing we'll get to this when we work on tracking downthe context menu test failures. > > @@ +127,5 @@ > > + textLength = newTextLength; > > + return false; > > + } > > + return true; > > + }, 45000, 1000); > > Wow, that's a huge timeout! :) Can we reduce it? I'd like to give it the extra time, it's a rather large drag. When these ran in debug builds it definitely needed it.
(In reply to Rodrigo Silveira [:rsilveira] from comment #41) > Comment on attachment 738640 [details] [diff] [review] > sub frame content tests v.1 > > Review of attachment 738640 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/metro/base/tests/mochitest/browser_selection_frame_content.js > @@ +121,5 @@ > > + > > + let scrollPromise = waitForEvent(gFrame.contentDocument.defaultView, "scroll"); > > + gFrame.contentDocument.defaultView.scrollBy(0, 200); > > + yield scrollPromise; > > + ok(scrollPromise && !(scrollPromise instanceof Error), "scrollPromise error"); > > Nit: not needed, yield will throw when there is an error. Same bellow. > > @@ +148,5 @@ > > + yield popupPromise; > > + ok(popupPromise && !(popupPromise instanceof Error), "promise error"); > > + > > + // copy text routes through message manager > > + yield waitForMs(500); > > You can use a waitForCondition like in > https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/ > mochitest/browser_context_menu_tests.js?rev=37fd7284be3a#77 updated, thanks!
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: