Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#15174 closed task (fixed)

dojox.mobile: updates to DOH tests

Reported by: Adrian Vasiliu Owned by: ykami
Priority: undecided Milestone: 1.8
Component: DojoX Mobile Version:
Keywords: Cc: Hikaru Tamura, cjolif, Eric Durocher
Blocked By: Blocking:

Description

In trunk (Dojo 1.8), dojox.mobile's DOH tests need a number of updates to solve failures in various browsers. The attached patch solves failures due to sizes of various elements which are slightly different than in the past when running the current trunk in Chrome 18, Firefox 11, and IE9. Additional fixes are needed for running on mobile devices. I will reattach a new patch merging all changes (so the commit can wait for the final patch).

Attachments (1)

patch15174.patch (15.6 KB) - added by Adrian Vasiliu 7 years ago.
DOH tests: updates for IE (updated dimensional values for IE9 and skip on older IE); avoid checking ContentPane? too early (with new xhr the loading order is undetermined); added test cases for Spinner with specified initial value; added a README with some explanations; now producing zero error/failure on Chrome 19, FF13, IE8 and IE9. - Adrian Vasiliu, IBM, CCLA

Download all attachments as: .zip

Change History (21)

comment:1 Changed 8 years ago by Adrian Vasiliu

I'm wondering whether the DOH tests should continue to check precise size/position numbers. Given that many of the actual results are different depending on the browser, and seem to evolve in time, this makes the tests hard to maintain. Maybe at least in some cases the test could avoid checking precise numbers and check instead some higher-level requirements. Anyway, this is just food for thoughts.

comment:2 Changed 8 years ago by ykami

Cc: Hikaru Tamura added

Adrian, thank you for the patch. But currently we do not see any DOH failures even on Chrome 18. Conversely, the DOH broke when we applied your patch. We've been testing on English Windows and Japanese Windows and getting the same results. Not sure what makes the difference, but perhaps we better relax some of the tests that check pixels to make them more robust.

comment:3 Changed 8 years ago by Adrian Vasiliu

Well, on my side I did get failures when running against dojo-trunk (before my patch) in Chrome 18 on an English Windows 7 (thinkpad). Okay, I will relax some of the tests. You say the DOH broke with the patch applied, was there any error message? On my side it wasn't broken at least in Chrome 18, FF11 and IE9, using the dojo-trunk updated at that time. I will come back to the DOH tests when I'll be free of other higher priority work.

comment:4 Changed 7 years ago by Adrian Vasiliu

Running the unpatched DOH tests (dojo-trunk version as of today), the results are as follows:

Chrome 19: 1 errors, 12 failures

FF 13: 0 errors, 1 failures

IE9: 0 errors, 6 failures

I've attached a new version of my patch for the DOH tests which now produces no error/failure on these 3 browsers.

comment:5 Changed 7 years ago by cjolif

Cc: cjolif added

comment:6 Changed 7 years ago by bill

FYI, I tried on Chrome (windows V20) without the patch and I got a few failures:

EdgeToEdge category:

    Error: test timeout in ../../dojox/mobile/tests/doh/EdgeToEdgeCategory.html::dojox.mobile.test.doh.EdgeToEdgeList::EdgeToEdgeList Verification2
     ERROR IN:
 		 function (){
				var d = new doh.Deferred();
				var demoWidget = dijit.byId("dojox_mobile_EdgeToEdgeList_0");
				demoWidget.set({transition :"flip"});
				doh.assertEqual("flip", demoWidget.get("transition"));
				demoWidget.set({transition :"fade"});
				doh.assertEqual("fade", demoWidget.get("transition"));

				fireOnMouseDown("item3");
				fireOnMouseUp("item3");
				var view = dijit.byId("foo");
				dojo.connect(view, "onAfterTransitionOut", this, d.getTestCallback(function(){
					var demoWidget = dijit.byId("dojox_mobile_EdgeToEdgeCategory_0");
					doh.assertEqual('mblEdgeToEdgeCategory', demoWidget.domNode.className);
					doh.assertEqual('Applications', demoWidget.domNode.innerHTML);

					demoWidget = dijit.byId("dojox_mobile_EdgeToEdgeList_1");
					doh.assertEqual('mblEdgeToEdgeList', demoWidget.domNode.className);

					verifyListItem("dojox_mobile_ListItem_0", 'Video', 'Off', "", false, false, false);
					verifyListItem("dojox_mobile_ListItem_1", 'Maps', 'VPN', "", true, false, false);
					verifyListItem("dojox_mobile_ListItem_2", 'Phone Number', 'Off', "", false, false, false);
				}));
				return d;
			}
 FAILED test: ../../dojox/mobile/tests/doh/EdgeToEdgeCategory.html::dojox.mobile.test.doh.EdgeToEdgeList::EdgeToEdgeList Verification2 59332 ms

ContentPane:

GROUP "dojox.mobile.test.doh.ContentPane" has 2 tests to run
     _AssertFailure: assertNotEqual() failed: not expected |null| but got |undefined| with hint: 
 		ContentPane: Did not instantiate first child. id=view1-pane4
     ERROR IN:
 		 function (){
						var widget = registry.byId("view1-pane14");
						var d = new runner.Deferred();
						var handle1 = connect.connect(widget, "loadHandler", this, d.getTestCallback(function(){
							connect.disconnect(handle1);
							_assertCorrectContentPanes(["view1-pane1","view1-pane2","view1-pane3","view1-pane4","view1-pane5"]);
							_assertCorrectContentPanes(["view1-pane6","view1-pane7","view1-pane8","view1-pane9","view1-pane10"]);
							_assertCorrectContentPanes(["view1-pane11","view1-pane12","view1-pane13","view1-pane14","view1-pane15"]);
						}));
						return d;
					}
 FAILED test: ../../dojox/mobile/tests/doh/ContentPaneTests.html::dojox.mobile.test.doh.ContentPane::ContentPane Verification1 70 ms
     Error: test timeout in ../../dojox/mobile/tests/doh/ContentPaneTests.html::dojox.mobile.test.doh.ContentPane::ContentPane Verification2
     ERROR IN:
 		 function (){
						var widget = registry.byId("view2-pane14");
						var d = new runner.Deferred();
						var handle2 = connect.connect(widget, "loadHandler", this, d.getTestCallback(function(){
							connect.disconnect(handle2);
							_assertCorrectContentPanes(["view2-pane1","view2-pane2","view2-pane3","view2-pane4","view2-pane5"]);
							_assertCorrectContentPanes(["view2-pane6","view2-pane7","view2-pane8","view2-pane9","view2-pane10"]);
							_assertCorrectContentPanes(["view2-pane11","view2-pane12","view2-pane13","view2-pane14","view2-pane15"]);
						}));
						_showView2();
						return d;
					}
 FAILED test: ../../dojox/mobile/tests/doh/ContentPaneTests.html::dojox.mobile.test.doh.ContentPane::ContentPane Verification2 4010 ms

However, when I ran the tests again they all passed.

comment:7 Changed 7 years ago by cjolif

I just got the ContentPane error and indeed it fires only the first time (probably because the referenced file is already in cache the second time so due to timing conditions the error does not appear anymore). I don't have any other errors. So before committing this I would like to better understand why the other changes are needed. On the ContentPane side I confirm the new test is fixing the issue however I see in the patch that we don't fully understand what's going on ("CHECKME"). Maybe we should before modifying the test?

comment:8 Changed 7 years ago by cjolif

Cc: Eric Durocher added

comment:9 Changed 7 years ago by Adrian Vasiliu

Yes, an attempt to understand it is ongoing. For ContentPane, the story is the following: the test creates and fills the ContentPane programmatically (for the failing case, with a "href"), then it used to check on the "loadHandler" event that the pane has a child. But this appears to be too early. I've tried to listen to "onLoad" instead of "loadHandler", but the behavior is the same. What did work is doing the checks in a setTimeout once getting the "loadHandler". So what needs to be clarified is the following: is the DOM supposed to be already filled when "loadHandler" (or "onLoad") are fired?

Will also try to understand what's going on with the other tests that failed on my side (without the patch) while didn't on yours (in particular on Christophe's).

Last edited 7 years ago by Adrian Vasiliu (previous) (diff)

comment:10 Changed 7 years ago by Adrian Vasiliu

Besides the partially random behavior of the ContentPane test before the patch, the results were different on Chrome because... of browser's zoom (thanks edurocher for the good intuition!). I've attached a new version of the patch. (Produces zero error/failure in Chrome 19, FF 13, and IE9 - tested after insuring browser zoom=1:1.)

comment:11 Changed 7 years ago by ykami

I confirmed the ContentPane failures. I also confirmed they did not happen in my one month old snapshot of the trunk. Did you figure out what made the difference?

comment:12 Changed 7 years ago by Adrian Vasiliu

What I can say for now is that the old version of the test passes okay with the current Dojo trunk if I revert dojo/_base/xhr.js at revision 28611 (5 weeks ago). So the recent massive changes in xhr seems to be involved in this change of behavior of our ContentPane. Next step for me (right now) is to try to isolate and reproduce the issue in a minimal test case such that I can enter a defect (and maybe patch) for the new xhr. The issue being that there should still be a way to be notified when xhr's job is fully completed.

Last edited 7 years ago by Adrian Vasiliu (previous) (diff)

comment:13 Changed 7 years ago by Eric Durocher

I investigated a little too, I think what causes the test to fail is that the new xhr implementation seems to call the load callbacks in a different order. The test relied on the fact that the last loaded content was for the view1-pane14 pane, and it only listens to the loadHandler of that pane to check everything. With the new xhr implementation, view1-pane14 is sometimes loaded before view1-pane4, and so at the time of the checks view1-pane4 is not yet loaded, thus the error.

I verified this by adding a console log in dojox/mobile/ContentPane.loadHandler. You can see that with the old XHR, view1-pane14 is always loaded last, whereas with the new implementation the order is different (and can vary from one run to another).

I don't think we can consider this a real regression of dojo/_base/xhr, but maybe I am wrong and there is actually a specified order? If not, the ContentPane test should not rely on a particular order (maybe it should wait for all loadHandlers to be called?).

comment:14 Changed 7 years ago by cjolif

Thanke Eric for your analysis. I agree this does not sound like a regression but more like a test relying on a non specified behavior. So the test should indeed be re-written to not rely on the order on which the pane are actually loaded.

comment:15 Changed 7 years ago by ykami

Thanks Adrian and edurocher for your investigation. Then I think the setTimeout fix is ok. I just didn't want to hide the root cause without knowing what's going on.

For other fixes, unfortunately, your patch didn't fix the ExpandingTextArea/TextArea errors on my IE8. I got errors as below.

 GROUP "dojox.mobile.test.doh.ExpandingTextArea" has 2 tests to run     _AssertFailure: assertEqual() failed: 	expected
		25px
	but got
		26px
GROUP "dojox.mobile.test.doh.TextArea" has 2 tests to run     _AssertFailure: assertEqual() failed: 	expected
		55
	but got
		58

Should we skip these tests when on IE? I'm afraid these two tests are too fragile.

comment:16 Changed 7 years ago by Adrian Vasiliu

edurocher: thanks a lot for you lights and sorry for having been so blind...

ykami: a) Concerning the IE issues: my plan was to give the priority to IE9 over older IE. And the patch includes an added README with this note: "The dimensional checks performed by a few tests are calibrated for execution in IE9, hence may fail in older IE versions. This does not mean the tested widget is broken in the older IE; it only means such tests do not hold for older IE versions." (also included a note about the browser zoom which needs to be at 1:1). Now, of course we can also decide to simply skip these tests when on IE. I let erudorcher / cjolif decide.

b) Concerning the ContentPane - now that we know the reason of the failure, maybe we could replace the setTimeout trick by counting the number of calls of loadHandler (separately for each view) and doing the checks after the last call. However this would require to take care when adding more test cases. Not sure it's worth, so maybe the setTimeout it's good enough.

comment:17 Changed 7 years ago by ykami

ok, then, let's skip those two tests when on IE8 or older. As for the ContentPane, I think setTimeout is good enough considering the purpose of the tests.

comment:18 Changed 7 years ago by Eric Durocher

Yes I agree, setTimeout is good enough.

Last edited 7 years ago by Eric Durocher (previous) (diff)

Changed 7 years ago by Adrian Vasiliu

Attachment: patch15174.patch added

DOH tests: updates for IE (updated dimensional values for IE9 and skip on older IE); avoid checking ContentPane? too early (with new xhr the loading order is undetermined); added test cases for Spinner with specified initial value; added a README with some explanations; now producing zero error/failure on Chrome 19, FF13, IE8 and IE9. - Adrian Vasiliu, IBM, CCLA

comment:19 Changed 7 years ago by cjolif

Resolution: fixed
Status: newclosed

fixed in [29129]

comment:20 Changed 7 years ago by cjolif

Milestone: tbd1.8
Note: See TracTickets for help on using tickets.