Closed Bug 850559 Opened 13 years ago Closed 13 years ago

test_imestate.html needs to call restoreNormalRefresh after advanceTimeAndRefresh

Categories

(Core :: Widget, defect)

Product:
Core
Component:
Widget
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(2 files, 3 obsolete files)

Fix tests that call advanceTimeAndRefresh without calling restoreNormalRefresh.
1.95 KB, patch
vlad
: review+
Details | Diff | Splinter Review
Make mochitests check that the test didn't leave the refresh driver under test control.
5.36 KB, patch
ted
: review+
Details | Diff | Splinter Review
http://hg.mozilla.org/mozilla-central/file/1b49fb552e18/dom/interfaces/base/nsIDOMWindowUtils.idl#l1130 The symptoms that I was seeing were that a subsequent mochitest could not open a panel during a load event. When it tried to open a panel during the load event, the mochitest iframe continued to contain the previous mochitest document. This was not just failure to paint the new document as expected from refresh driver issues, because text inputs in the previous test could be selected with the mouse and displayed a cursor.
Attachment #724307 - Flags: review?(masayuki)
Blocks: 835044
Hmm, I don't familiar with the symptom and the method which you are adding. Could you request review to somebody who is familiar with the method?
Attachment #724307 - Flags: review?(masayuki) → review?(dbaron)
Masayuki, can you explain why this test calls advanceTimeAndRefresh(), please? (I expect that won't be obvious to David, and I don't know)
Flags: needinfo?(masayuki)
Comment on attachment 724307 [details] [diff] [review] restoreNormalRefresh after advanceTimeAndRefresh This is not the only test with this problem. They seem to be: content/events/test/test_bug574663.html content/events/test/window_wheel_default_action.html widget/tests/test_imestate.html r=dbaron on fixing this. I will disable any of these tests not fixed within 24 hours.
Attachment #724307 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #4) > I will disable any of these tests not fixed within 24 hours. To be clear, this is because these mess with later tests in a really bad way. That said, I probably shouldn't have said that -- it's trivial to fix, and it should just be fixed, asap.
And maybe we should rename advanceTimeAndRefresh to something scarier like controlRefreshTimeAdvanceAndRefresh?
or controlRefreshTimeAndAdvanceAndRefresh?
Attachment #724713 - Attachment is obsolete: true
Attachment #724713 - Flags: review?(karlt)
er, I wish bzexport didn't change assignee by default
Assignee: dbaron → karlt
Comment on attachment 724714 [details] [diff] [review] Assert when a test leaves test control of refresh driver on. Looks fine to me, but, if we do this, then we need to restoreNormalRefresh() in test_imestate.html before runEditableSubframeTests().
Attachment #724714 - Flags: review?(karlt) → review+
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #7) > And maybe we should rename advanceTimeAndRefresh to something scarier like > controlRefreshTimeAdvanceAndRefresh? Fine, or perhaps something like advanceTimeRefreshAndPause?
(In reply to Karl Tomlinson (:karlt) from comment #3) > Masayuki, can you explain why this test calls advanceTimeAndRefresh(), > please? > (I expect that won't be obvious to David, and I don't know) I'm not sure. I probably stole the method from another test. I don't know the method what's doing. So, if you think it's not necessary, please remove it.
Flags: needinfo?(masayuki)
Mac and GTK tests pass (ignore the new test_panel_mouse_coords.xul I was testing) without advanceTimeAndRefresh(). https://tbpl.mozilla.org/?tree=Try&rev=4decd1d97fc1 Waiting for WINNT results: https://tbpl.mozilla.org/?tree=Try&rev=623e7a0fc63e
Comment on attachment 724714 [details] [diff] [review] Assert when a test leaves test control of refresh driver on. I think this approach isn't practical; I'm testing a patch to make mochitest report test failures if a test leaves things under refresh driver control (and also fix the situation).
Attachment #724714 - Attachment is obsolete: true
Comment on attachment 724307 [details] [diff] [review] restoreNormalRefresh after advanceTimeAndRefresh Removed advanceTimeAndRefresh from test_imestate.html https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0eadfc2075
Attachment #724307 - Attachment is obsolete: true
Whiteboard: [leave open]
Comment on attachment 724307 [details] [diff] [review] restoreNormalRefresh after advanceTimeAndRefresh Please don't obsolete the patch because it was checked in.
Attachment #724307 - Attachment is obsolete: false
Attachment #724307 - Flags: checkin+
Comment on attachment 724307 [details] [diff] [review] restoreNormalRefresh after advanceTimeAndRefresh Er, sorry, never mind.
Attachment #724307 - Attachment is obsolete: true
Attachment #724307 - Flags: checkin+
Try push with the mochitest patch and the other patches here: https://tbpl.mozilla.org/?tree=Try&rev=abf260e55a65 Try push with just the mochitest patch without the patches in this bug: https://tbpl.mozilla.org/?tree=Try&rev=b7e5a29ba76e
Comment on attachment 725444 [details] [diff] [review] Make mochitests check that the test didn't leave the refresh driver under test control. Review of attachment 725444 [details] [diff] [review]: ----------------------------------------------------------------- This is fine, but should we just add a more user-friendly API to the test harness that automatically cleans this up when the test is finished?
Attachment #725444 - Flags: review?(ted) → review+
This patch actually does clean things up, but it also makes the test fail -- I'd rather not have tests that use these APIs without understanding what they are (as happened here).
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #23) > Try push with the mochitest patch and the other patches here: > https://tbpl.mozilla.org/?tree=Try&rev=abf260e55a65 > > Try push with just the mochitest patch without the patches in this bug: > https://tbpl.mozilla.org/?tree=Try&rev=b7e5a29ba76e The first of these was green; the only failure in the second was: 12:07:48 INFO - 42069 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_imestate.html | test left refresh driver under test control I'm not sure why the tests in attachment 724715 [details] [diff] [review] weren't causing failures there as well.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #26) > I'm not sure why the tests in attachment 724715 [details] [diff] [review] > weren't causing failures there as well. I guess it's because both are running in a separate *window*. But that could change if we change what has a separate refresh driver and what shares a refresh driver.
I've not understood this bug's fix yet, however, I want to say, thank you for the fix. And I apologize that I couldn't touch this bug due to working on other bugs. I'll check this bug's change for new tests which I will write. Thank you.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: