Closed Bug 508710 Opened 15 years ago Closed 13 years ago

moving toolkit/content/tests/widgets/*.xul from mochitest to mochitest-chrome?

Categories

(Toolkit :: UI Widgets, defect)

Product:
Toolkit
Component:
UI Widgets
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla7

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

(Whiteboard: [specialpowers][inbound])

Attachments

(1 file, 5 obsolete files)

move most toolkit/content/tests/widgets/* to chrome (6.0)
109.64 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review 
In testing Fennec, I have noticed a lot of issues with the toolkit/content/tests/widgets/*.xul tests hanging or not even running.  This brought up a question of why are these xul tests in mochitest and not mochitest-chrome?

It seems like everything but the video related tests should live in mochitest-chrome.

Is this something we can move between harnesses?  Is there a reason why this is in mochitest?  Moving this would help out with Fennec testing greatly as well as keep things logically organized better.  I suspect it could help with some of the random orange stuff as well.
Whiteboard: [specialpowers]
this patch isn't complete, but is green if you ignore the 7 test files that fail:
test_arrowpanel.xul - passes standalone, fails in the harness.xul wrapper
test_deck.xul - passes standalone, fails in the harness.xul wrapper
test_mouse*.xul (3) - needs more attention
test_contextmenu_nested.xul - causes the tests to hang, fails to open menu on second test via mouseover
test_menulist_null_value - serious WTF.  causes the tests to restart half way through, sometimes crashes the browser.  

fixing these called out tests will allow us to migrate.  Another solution is to migrate everything but these tests until we resolve them.
from the $(objdir)/_tests/testing/mochitest running:
python runtests.py --chrome --autorun --test-path=toolkit/content/tests/widgets

will result in a continuing reloading of the browser.

removing test_deck.xul from the test list will go until test_popupincontext.xul, then reload/crash.

Alternatively, removing test_tree_hier_cell.xul and test_tree_column_reorder.xul will allow all the tests (including test_deck.xul and test_popup_incontext.xul) to pass (at least locally on linux 5 times in a row)
Congratulations! You found a datepicker bug! Compare:

> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/datetimepicker.xml#1225

> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scale.xml#188

It appears that trees and listboxes also suffer from the same bug, while menulists and tabboxes also appear to be free of the bug.
I am not an expert of this stuff (and I don't really understand the bug), but I am glad you were able to determine there is a bug.

What can I do to help get this fixed?  This will probably solve a handful of random issues I have been seeing.

My next set of issues I discovered today are related to timing.  Specifically the tests that start with a onload="setTimeout(func, 0)".  Adjusting this to a 500ms delay appears to solve a few of the random problems I saw in test_popupremoving.xul/test_popupremoving_frame.xul and a few other widget .xul files.
Blocks: 462483
Depends on: 655004
(In reply to comment #3)
> Congratulations! You found a datepicker bug! Compare:
> 
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/datetimepicker.xml#1225
> 
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scale.xml#188

I can't really tell what difference you're highlighting here, can you explain further?
(In reply to comment #5)
> (In reply to comment #3)
> > Congratulations! You found a datepicker bug! Compare:
> > 
> > > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/datetimepicker.xml#1225
> > 
> > > http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scale.xml#188
> 
> I can't really tell what difference you're highlighting here, can you
> explain further?

Sure. Some of the tests test the operation of Page Up and Page Down on date pickers. These are normally used in unscrollable XUL documents so you don't notice that the event bubbles up to the root frame. But in the tests, the event bubbles up to the harness causing it to scroll. This is not a problem for the datepicker test, but then subsequent tests focus elements in the test causing the harness to scroll down slightly to reveal the elements in question. However one of the tests then tries to click an element at the top of the window. Because of the scrolling, the element is not there, it is hidden under the "Run Tests" button. Thus the test ends up running the tests again. Oops.

I don't know why the test passes under the regular mochitest harness.
Presumably since then they're loaded from a http URL inside an iframe inside a <browser> in the normal browser UI, so they can't reach out to touch the harness. (Or they can't screw things up in quite this way.)
is there a way to force the events not to bubble up?  We are loading the test file in an iframe:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/harness-overlay.xul#98

I am a bit lost on how to approach fixing these tests.
(In reply to comment #8)
> is there a way to force the events not to bubble up?

Yes, fix the widgets to stop the events from bubbling after they've been handled
(In reply to comment #8)
> is there a way to force the events not to bubble up?  We are loading the
> test file in an iframe:
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/harness-
> overlay.xul#98
> 
> I am a bit lost on how to approach fixing these tests.

I think what we need to do here is to call the stopPropagation method on the event once the control handles it.  
* https://developer.mozilla.org/en/xul_event_propagation#Event_Bubbling
* https://developer.mozilla.org/en/XUL_Tutorial/More_Event_Handlers
Attached patch partial move of tests (3.0) (obsolete) — — Splinter Review 
move part of the tests.  The tests that are not moved need original test authors to make these work.
Assignee: nobody → jmaher
Attachment #528692 - Attachment is obsolete: true
I am trying to move all of these tests from mochitest to mochitest-chrome as part of our effort to remove enablePrivilege and gets tests in the correct harnesses.  I am moving the majority of the tests in the toolkit/content/tests/widgets/ directory and below is a list of all the original test authors (with a few exceptions).  Can each one of you take a minute or two to look at your original test and let me know if you think there are any issues with moving it to a chrome test?

bz:
test_bug359754.xul

mano:
test_menu_hide.xul
test_bug360220.xul
test_bug382990.xul

wladimir:
test_bug365773.xul

mossop:
test_bug457632.xul
test_bug509732.xul
test_hiddenitems.xul
test_hiddenpaging.xul

arno:
test_bug460942.xul

dao:
test_bug554279.xul
test_textbox_search.xul
test_textbox_empty.xul
test_toolbar.xul

felipe:
test_bug557987.xul
test_bug562664.xul

surkov:
test_menu.xul

mstange:
test_menuitem_blink.xul
test_scrollbar.xul

florian:
test_menulist_null_value.xul

neil:
test_button.xul
test_closemenu_attribute.xul
test_colorpicker_popup.xul
test_contextmenu_list.xul
test_focus_anons.xul
test_menu_hide.xul
test_menulist.xul
test_menulist_keynav.xul
test_notificationbox.xul
test_panelfrommenu.xul
test_popup_attribute.xul
test_popup_coords.xul
test_popup_keys.xul
test_popup_preventdefault.xul
test_popup_scaled.xul
test_popup_tree.xul
test_popup_recreate.xul
test_popuphidden.xul
test_popupincontent.xul
test_popupremoving.xul
test_popupremoving_frame.xul
test_position.xul
test_progressmeter.xul
test_props.xul
test_radio.xul
test_scale.xul
test_sorttemplate.xul
test_statusbar.xul
test_richlist_direction.xul
test_tabbox.xul
test_tabindex.xul
test_textbox_number.xul
test_timepicker.xul
test_tooltip.xul
test_noautohide.xul
test_tree.xul
test_tree_hier.xul
test_tree_hier_cell.xul
test_tree_single.xul
test_tree_view.xul
test_tree_column_reorder.xul
(In reply to comment #12)
> Can each one of you take a
> minute or two to look at your original test and let me know if you think
> there are any issues with moving it to a chrome test?

What sorts of issues should we be looking for?
issues would be that if it is run as a chrome test we wouldn't be testing what the test was written for originally.  If the test is accessing anything in content directly, there might be issues.  

Then again I have ran these on try server many times and they seem to be passing and generating the same number of test passes as they do in mochitest-plain.

thanks!!
(In reply to comment #12)
> mossop:
> test_bug457632.xul
> test_bug509732.xul
> test_hiddenitems.xul
> test_hiddenpaging.xul

These look fine
(In reply to comment #12)
> test_menuitem_blink.xul
> test_scrollbar.xul

no problems here
> test_bug359754.xul

Should be fine.
(In reply to comment #12)
> dao:
> test_bug554279.xul
> test_textbox_search.xul
> test_textbox_empty.xul
> test_toolbar.xul

Should be ok. (I'm not the author of test_toolbar.xul, I just modified it twice.)
> felipe:
> test_bug557987.xul
> test_bug562664.xul  [actually test_bug562554.xul]

should be fine
(In reply to comment #12)
> wladimir:
> test_bug365773.xul

Should be fine.
> test_bug359754.xul

This test is redundant and can be removed.

The tests test_popup_attribute.xul test_popup_button.xul and test_tooltip.xul rely on not being able to access document.popupNode and document.tooltipNode from a different domain (http://sectest2.example.org:80 is used in these tests), which could be affected by changing to a chrome test.

The other tests are ok.
(In reply to comment #12)

> florian:
> test_menulist_null_value.xul

Should be fine.
(In reply to comment #12)
> surkov:
> test_menu.xul

The tricky part with this one is that it requires the accessibility engine to be invoked. That's easy enough, but once you enable accessibility it is "on" until the browser is closed. I'm not sure if we want accessibility enabled for the remainder of the chrome tests that would follow this one (I presume the test file list if alphabetical or something).

I would like to be able to add and run chrome accessibility tests, so this is worth figuring out.

Alexander what do you think?

(Feel free to file a spin off if you think we'll hash this out over bugzilla)
(In reply to comment #23)

> Alexander what do you think?

You were confused with files names.

> surkov:
> test_menu.xul

should be fine, all it does is tests XBL methods for XUL menus. However it's worth to double check this, for example, with Neil.
This patch was green on try server May 26th, but on June 6th, it failed on all OSX builds.  At first I thought it was test_tree_hier.xul (1 failure) and test_tree_hier_cell.xul (timed out), but removing those from mochitest-chrome, I saw plenty of other errors in a few other test files.

Something much have changed on the OSX platform in the week that I was afk.  If anybody has any ideas, that would be helpful.
patch to move all but 12 of the tests.  This passes on try server.
Attachment #529106 - Attachment is obsolete: true
Attachment #535640 - Attachment is obsolete: true
Attachment #540366 - Flags: review?(enndeakin)
It's a little confusing to change these to chrome tests, but leave them in the 'widgets' directory.

You've also missed changing the script urls in a number of files (test_deck.xul, window_popup_button.xul were two of possibly others that I noticed)
that patch leaves 12 files in mochitest-plain for now.  The problem is those tests have been problematic over time and we will need to tackle those in the next patch.  from what I can tell all the tests+files are in the right places (chrome/tests).

would you rather I create a chrome/ subdir and move the tests+files there?
I meant that you've left the source files in the toolkit/content/tests/widgets directory rather than the toolkit/content/tests/chrome directory. Having them scattered about might be confusing even if they do plan on all moving over eventually.
all migrated tests and related files are in the chrome/ subdir now.  In addition there are test and data/script files in the regular directory.  These are all needed by the 12 tests that were problematic at one point or time and we will work on moving after this patch is done.
Attachment #540366 - Attachment is obsolete: true
Attachment #540709 - Flags: review?(enndeakin)
Attachment #540366 - Flags: review?(enndeakin)
Neil, can you take a look at this latest patch?
You've removed the test_contextmenu_nested.xul and test_tooltip_noautohide.xul tests from the Makefile, but haven't added them anywhere.

Does the test_popup_button.xul test fail in some way when moved to chrome?

Why change the stylesheet path in test_menulist_null_value.xul?

I actually meant to move the source files into the 'chrome' directory that already exists.
updated patch to move test_tooltip_noautohide.xul and test_popup_button.xul to chrome.  Also fixed a typo in test_menulist_null_value.xul and added test_contextmenu_nested.xul to the mochitest-plain tests.  

All chrome tests now live in: toolkit/content/tests/chrome instead of toolkit/content/tests/widgets.

The remaining .xul test files in the widgets/ directory should be looked at after landing this.
Attachment #540709 - Attachment is obsolete: true
Attachment #541392 - Flags: review?(enndeakin)
Attachment #540709 - Flags: review?(enndeakin)
Attachment #541392 - Flags: review?(enndeakin) → review+
Whiteboard: [specialpowers] → [specialpowers][inbound]
http://hg.mozilla.org/mozilla-central/rev/c65f1fb0449d
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: