Closed Bug 367393 Opened 18 years ago Closed 13 years ago

Replace the packed MochiKit dependencies in mochitest harness.

Categories

(Testing :: Mochitest, defect)

Product:
Testing
Component:
Mochitest
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: sayrer, Assigned: mdas)

References

(Depends on 1 open bug)

Details

(Whiteboard: [buildfaster:p1])

Attachments

(6 files, 20 obsolete files)

Update the packed.js dependent tests [Checked in: Comment 53]
831.07 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
Remove packed.js script tags from mochiplain tests (first half) [Checked in: Comment 52]
2.81 MB, patch
mdas
: review+
Details | Diff | Splinter Review
Remove packed.js script tags from mochiplain tests (second half) [Checked in: Comment 51]
492.78 KB, patch
mdas
: review+
Details | Diff | Splinter Review
Remove packed.js script tags from chrome and b-c tests [Checked in: Comment 54]
814.21 KB, patch
mdas
: review+
Details | Diff | Splinter Review
Remove packed.js dependencies in the harnesses [Checked in: Comment 55]
39.57 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
removing packed.js from new tests [Checked in: Comment 48]
22.22 KB, patch
mdas
: review+
Details | Diff | Splinter Review
replace packed.js with this, and include a packed_full.js file if a few tests need something extra.
Moving a bunch of Core :: Testing bugs to Testing :: Mochitest to clear out the former, which is obsolete now that we have more specialized categories for such bugs; filter on the string "MochitestMmMm" to delete all these notifications.
Component: Testing → Mochitest
Product: Core → Testing
QA Contact: testing → mochitest
Version: Trunk → unspecified
I'm not convinced we need packed.js at all. In some profiling runs from :bz and I, we can see that we'll save quite a bit of time by removing packed.js from the mochitests. I've verified that we could save as much as five minutes off a debug test run (tested on windows and linux), and I think Boris was using opt builds when he wrote the following in dev.planning: On 5/30/11 7:22 PM, Robert O'Callahan wrote: > Has anyone done any profiling of general mochitest runs? I just did a bit... Running mochitest near the beginning of the run, sampled for 10s.... 90% of the time is in mach_msg_trap (basically idle time, as far as I can tell). That matches my CPU meter, actually; it never went about 25% for that process. > I wonder how much time is spent loading and parsing MochiKit/packed.js > (150K) into every single mochitest, or even SimpleTest/SimpleTest.js (28K). This testcase: <!DOCTYPE html> <base href="http://mochi.test:8888/"> <script> var s = new Date(); </script> <script type="text/javascript" src="/MochiKit/packed.js"></script> <script> var m = new Date(); </script> <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> <script> var e = new Date(); alert("packed: " + (m - s) + ", SimpleTest:" + (e - m)); </script> shows numbers on the order of 42ms and 1ms respectively on my machine (a year-old MBP) when loaded in the a browser that's supposed to be running mochitest. We have order of 3600 mochitest files, so the total overhead of packed.js over the whole run is about 2.5 mins. > or we could even look at doing some kind of optimization so that when we load a > cached script we reuse the bytecode or something. https://bugzilla.mozilla.org/show_bug.cgi?id=288473 -Boris So I'd advocate just trying to remove packed.js entirely from the mochitest-plain tests and seeing what happens. If we end up needing a few functions from it, we can put them into simpletest.js. And as a next optimization we can look into making a simpletest.js packed file. But first, let's kill packed.js. And then we can file bugs for next steps. I think Mdas said she'd look into this, CC'ing her.
Whiteboard: [buildfaster:p1]
> and I think Boris was using opt builds when he wrote the following in > dev.planning Yes. Habit of not testing performance stuff in debug builds, though of course this is one of those rare cases when we do care about debug build performance. I can definitely believe that parsing packed.js could take 5 mins over the course of a test run in a debug build.
Assignee: sayrer → nobody
Assignee: nobody → mdas
Remove packed.js specific calls from the harnesses. Pulled out useful methods from MochiKit and added modified versions of them in SimpleTest.js. Added some of these helper methods in TestRunner.js and EventUtils.js. Replaced MochiKit's Logger with a LogController (new object).
Attachment #550479 - Flags: review?(jmaher)
Updated some of the tests that had dependencies on packed.js functions by using the SimpleTest.js versions where possible. Some tests had some other packed.js dependencies (like using the leaked i variable) that had to be fixed.
Attachment #550480 - Flags: review?(jmaher)
Attachment #550483 - Attachment is obsolete: true
Attachment #550484 - Flags: review?(jmaher)
split into two chunks
Attachment #550485 - Flags: review?(jmaher)
From the tests I ran in try, I can see that these patches reduced the runtimes of the debug builds' mochitests from anywhere between 3-7 minutes, but typically around 5 minutes, depending on the platform. The opt builds have also shown reductions from 10seconds to 1 minute, typically around the 30sec - 1 minute area.
Comment on attachment 550484 [details] [diff] [review] Remove packed.js script tags from mochiplain tests Review of attachment 550484 [details] [diff] [review]: ----------------------------------------------------------------- sorry for the r-, we have a few lines removed that look like they shouldn't and we really should remove the entire line (not add a newline). In many of the dom-level1/2 tests, it would be nice to add newlines to the big line of <script>...</script> tags, but that is a nice to have. ::: caps/tests/mochitest/test_bug246699.html @@ +4,5 @@ > https://bugzilla.mozilla.org/show_bug.cgi?id=246699 > --> > <head> > <title>Test for Bug 246699</title> > + please remove the newlines when you remove a line of code. ::: content/base/test/test_NodeIterator_basics_filters.xhtml @@ -37,5 @@ > 3, 8, // leading comment > 3, 1, // head > 3, 1, 3, // title > 3, 1, // first script tag > - 3, 1, // second script tag why is this removed? It looks like it is part of the test @@ -86,5 @@ > 8, // leading comment > 1, // head > 1, // title > 1, // first script tag > - 1, // second script tag why is this removed? according the comment this is part of the test ::: content/base/test/test_bug270145.xhtml @@ +51,5 @@ > </script> > </pre> > > </body> > +</html> what happened here? ::: content/base/test/test_bug424359-2.html @@ +311,5 @@ > > > </div> > </body> > +</html> what happened here? I think we lost a newline at the end of the file. ::: content/html/content/test/test_bug375003-2.html @@ +106,5 @@ > </script> > </pre> > > </body> > +</html> what happened here? this is showing a diff, maybe a missing newline? ::: dom/tests/mochitest/dom-level2-core/test_systemId01.html @@ +4,5 @@ > <META http-equiv="Content-Type" content="text/html; charset=UTF-8"> > <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> > <title>http://www.w3.org/2001/DOM-Test-Suite/level2/core/systemId01</title> > <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"> > +<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script><script type="text/javascript" src="DOMTestCase.js"></script><script type="text/javascript" src="exclusions.js"></script><script type="text/javascript"> can you add newlines here?
Attachment #550484 - Flags: review?(jmaher) → review-
(In reply to comment #12) > Comment on attachment 550484 [details] [diff] [review] [diff] [details] [review] > Remove packed.js script tags from mochiplain tests > > Review of attachment 550484 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > sorry for the r-, we have a few lines removed that look like they shouldn't > and we really should remove the entire line (not add a newline). In many of > the dom-level1/2 tests, it would be nice to add newlines to the big line of > <script>...</script> tags, but that is a nice to have. > > ::: caps/tests/mochitest/test_bug246699.html > @@ +4,5 @@ > > https://bugzilla.mozilla.org/show_bug.cgi?id=246699 > > --> > > <head> > > <title>Test for Bug 246699</title> > > + > > please remove the newlines when you remove a line of code. > > ::: content/base/test/test_NodeIterator_basics_filters.xhtml > @@ -37,5 @@ > > 3, 8, // leading comment > > 3, 1, // head > > 3, 1, 3, // title > > 3, 1, // first script tag > > - 3, 1, // second script tag > > why is this removed? It looks like it is part of the test > > @@ -86,5 @@ > > 8, // leading comment > > 1, // head > > 1, // title > > 1, // first script tag > > - 1, // second script tag > > why is this removed? according the comment this is part of the test > This is fine, FWIW. The test needs to know about each node in the document, but it doesn't hurt if one is removed. Another solution would be to just add an empty script element instead.
(In reply to comment #12) > Comment on attachment 550484 [details] [diff] [review] [diff] [details] [review] > Remove packed.js script tags from mochiplain tests > > Review of attachment 550484 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > sorry for the r-, we have a few lines removed that look like they shouldn't > and we really should remove the entire line (not add a newline). In many of > the dom-level1/2 tests, it would be nice to add newlines to the big line of > <script>...</script> tags, but that is a nice to have. > > ::: caps/tests/mochitest/test_bug246699.html > @@ +4,5 @@ > > https://bugzilla.mozilla.org/show_bug.cgi?id=246699 > > --> > > <head> > > <title>Test for Bug 246699</title> > > + > > please remove the newlines when you remove a line of code. I'll update the patches without the added newlines. > ::: content/base/test/test_NodeIterator_basics_filters.xhtml > @@ -37,5 @@ > > 3, 8, // leading comment > > 3, 1, // head > > 3, 1, 3, // title > > 3, 1, // first script tag > > - 3, 1, // second script tag > > why is this removed? It looks like it is part of the test > > @@ -86,5 @@ > > 8, // leading comment > > 1, // head > > 1, // title > > 1, // first script tag > > - 1, // second script tag > > why is this removed? according the comment this is part of the test Ms2ger is correct; that test needs to be updated if any node in the document is changed (that test iterates over each node and verifies it) > ::: content/base/test/test_bug270145.xhtml > @@ +51,5 @@ > > </script> > > </pre> > > > > </body> > > +</html> > > what happened here? > > ::: content/base/test/test_bug424359-2.html > @@ +311,5 @@ > > > > > > </div> > > </body> > > +</html> > > what happened here? I think we lost a newline at the end of the file. > > ::: content/html/content/test/test_bug375003-2.html > @@ +106,5 @@ > > </script> > > </pre> > > > > </body> > > +</html> > > what happened here? this is showing a diff, maybe a missing newline? I used a sed command over all files and it must have removed the newline as a byproduct. I'll fix that > ::: dom/tests/mochitest/dom-level2-core/test_systemId01.html > @@ +4,5 @@ > > <META http-equiv="Content-Type" content="text/html; charset=UTF-8"> > > <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> > > <title>http://www.w3.org/2001/DOM-Test-Suite/level2/core/systemId01</title> > > <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"> > > +<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script><script type="text/javascript" src="DOMTestCase.js"></script><script type="text/javascript" src="exclusions.js"></script><script type="text/javascript"> > > can you add newlines here? Expect to see it in the fixed patch!
Comment on attachment 550479 [details] [diff] [review] Remove packed.js dependencies in the harnesses Review of attachment 550479 [details] [diff] [review]: ----------------------------------------------------------------- It might be nice to create a mochikit_lite.js or something which has all the function we needed to rip out and use in the harness. It seems like that would reduce code duplication. marking as r+ as this looks good in general, just a lot of little nits to clean up. ::: dom/tests/mochitest/dom-level1-core/DOMTestCase.js @@ +687,5 @@ > + try { > + var tests = exposeTestFunctionNames(); > + for(var i = 0; i < tests.length; i++){ > + window[tests[i]](); > + } indentation needs fixed here @@ +689,5 @@ > + for(var i = 0; i < tests.length; i++){ > + window[tests[i]](); > + } > + } catch (ex) { > + ok(false, "Test threw exception: " + ex); in the other DOMTestCase.js we check if this is a known TODO and throw a todo. ::: dom/tests/mochitest/dom-level2-core/DOMTestCase.js @@ +690,5 @@ > try { > + var tests = exposeTestFunctionNames(); > + for(var i = 0; i < tests.length; i++){ > + window[tests[i]](); > + } indentation needs fixed here @@ +698,5 @@ > } else { > ok(false, "Test threw exception: " + ex); > } > } > +} why did this change? ::: dom/tests/mochitest/dom-level2-html/DOMTestCase.js @@ +690,5 @@ > try { > + var tests = exposeTestFunctionNames(); > + for(var i = 0; i < tests.length; i++){ > + window[tests[i]](); > + } indentation problem here. ::: testing/mochitest/tests/SimpleTest/EventUtils.js @@ +20,5 @@ > + return ((typeof(id) == "string") ? > + document.getElementById(id) : id); > +}; > + > +this.$ = this.getElement; why do you do this here? I see you replace a lot of $('...') calls elsewhere when you can do the same trick @@ +37,5 @@ > + this[path] = null; > + } catch (e) { > + // pass > + } > + } what are you doing here with 'once' ? ::: testing/mochitest/tests/SimpleTest/LogController.js @@ +1,2 @@ > +var LogController = {}; > + can we put a license header in this file? Also a few comments. It is mostly self explanatory. @@ +40,5 @@ > + for(var i = skip; i<args.length; i++){ > + ret.push(args[i]); > + } > + return ret; > +}; your }'s are indented too much @@ +66,5 @@ > + if (pair.ident != k || (pair[0] && !pair[0](msg))) > + continue; > + pair[1](msg); > + } > +}; this function isn't spaced the same as the rest of the file @@ +74,5 @@ > + filter = LogController.logLevelAtLeast(filter); > + } > + else { > + > + } why the blank else{} clause? Also spacing needs to be cleaned up here @@ +82,5 @@ > + }; > + > +LogController.removeListener = function(ident) { > + delete LogController.listeners[ident]; > + }; spacing and } need to be unindented a bit. ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +90,5 @@ > + } > + func(args); > + }; > + }; > +} can we get a little comment explaining what partial is supposed to do and who uses it? Also the condition blocks here have no whitespace between them and the {, please make this like the functions above and below. @@ +153,5 @@ > + window["onload"] = regfunc; > + } > + regfunc.callStack.push(func); > + }; > +} this is duplicated code in EventUtils.js? Is that necessary, or can we minimize the duplication? @@ +173,5 @@ > + if(html !== null && html !== undefined) > + //el.innerHTML = html; > + el.appendChild(document.createTextNode(html)); > + return el; > +} can you use the same whitespace as you do in other functions around the condition block in your if statements? @@ +195,5 @@ > + ).toLowerCase(); > + > + return style.getPropertyValue(selectorCase); > + }; > +} mixed use of whitespace on your condition blocks here @@ +368,5 @@ > /** > * Toggle element visibility > **/ > SimpleTest.toggle = function(el) { > + if (el.style && (el.style.display == 'block')) { why not use computedStyle from up above here? @@ +398,5 @@ > + } > + for(var t=0; t<elements.length; t++){ > + //TODO: again, for-in loop over elems seems to break this > + SimpleTest.toggle(elements[t]); > + } whitespace on condition block ::: testing/mochitest/tests/SimpleTest/TestRunner.js @@ +90,5 @@ > if (TestRunner._currentTest < TestRunner._urls.length) { > var runtime = new Date().valueOf() - TestRunner._currentTestStartTime; > if (runtime >= TestRunner.timeout * TestRunner._timeoutFactor) { > + var frameWindow = document.getElementById('testframe').contentWindow.wrappedJSObject || > + document.getElementById('testframe').contentWindow; why not create a function getElement() as you have in SimpleTest.js and EventUtils.js? @@ +111,5 @@ > return; > } > > + //TestRunner.deferred = callLater(30, TestRunner._checkForHangs); > + setTimeout(TestRunner._checkForHangs, 30000); should just remove the commented out line here
Attachment #550479 - Flags: review?(jmaher) → review+
Attachment #550484 - Attachment is obsolete: true
Attachment #551061 - Flags: review?(jmaher)
The new plaintest patches include changes to new tests.
Attachment #550485 - Attachment is obsolete: true
Attachment #550487 - Attachment is obsolete: true
Attachment #551066 - Flags: review?(jmaher)
Attachment #550485 - Flags: review?(jmaher)
Attachment #550487 - Flags: review?(jmaher)
Attachment #550486 - Attachment is obsolete: true
Attachment #551067 - Flags: review?(jmaher)
Attachment #550486 - Flags: review?(jmaher)
Comment on attachment 550480 [details] [diff] [review] Update the packed.js dependent tests Review of attachment 550480 [details] [diff] [review]: ----------------------------------------------------------------- overall this looks good. please cleanup or address my comments below before checking in. ::: browser/components/places/tests/browser/browser_library_batch_delete.js @@ +79,5 @@ > // Test live update. > PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId, > makeURI(TEST_URL), > PlacesUtils.bookmarks.DEFAULT_INDEX, > + "bm"); why is this 'i' removed? ::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_601667_filter_buttons.js @@ +91,5 @@ > ok(isChecked(menuItem), "menu item " + prefKey + " for category " + > aCategory + " is checked after clicking the button"); > ok(HUDService.filterPrefs[hudId][prefKey], prefKey + " messages are " + > "on after clicking the button"); > + menuItem = menuItem.nextSibling; why is this i++ removed? ::: content/html/content/test/test_bug100533.html @@ +8,5 @@ > <script type="text/javascript" src="/MochiKit/packed.js"></script> > + <script type="text/javascript" src="/MochiKit/Base.js"></script> > + <script type="text/javascript" src="/MochiKit/DOM.js"></script> > + <script type="text/javascript" src="/MochiKit/Style.js"></script> > + <script type="text/javascript" src="/MochiKit/Signal.js"></script> why are you not removing packed.js, but you are for the content/media/test/test_contentDuration*.html tests below? ::: content/media/test/test_delay_load.html @@ +101,5 @@ > // the video's finished loading (in the other document). > // Opening a new window to do this is a bit annoying, but if we use an iframe here, > // delaying of the iframe's load event might interfere with the firing of our load event > // in some confusing way. So it's simpler just to open another window. > +var w = window.open("", "testWindow", "width=400,height=400"); why did you remove the "+i" here? ::: dom/plugins/test/mochitest/test_npruntime_npnevaluate.html @@ +78,4 @@ > typeof(result) + "), expected " + json_expected + "(" + > + typeof(expected) + ")")); > + $("verbose").appendChild( > + createEl('br') how do you access $('verbose') here? In the previous file, we needed to do document.getElementById("testForm"). ::: dom/tests/mochitest/dom-level1-core/test_hc_attrremovechild2.html @@ +86,5 @@ > + if (testFails(o)) { > + failures.push(o); > + } > + } > + // shouldn't be 0 failures I don't understand why this specific test case is different from the other dom-level1-core tests? We are removing markTodos() from all the tests but this one? ::: testing/mochitest/tests/MochiKit-1.4.2/tests/test_MochiKit-Base.html @@ +4,4 @@ > <script type="text/javascript" src="../MochiKit/Base.js"></script> > <script type="text/javascript" src="../MochiKit/Iter.js"></script> > <script type="text/javascript" src="../MochiKit/DOM.js"></script> > <script type="text/javascript" src="../MochiKit/Style.js"></script> why is SimpleTest.js moved to the top here? This seems unnecessary? ::: testing/mochitest/tests/MochiKit-1.4.2/tests/test_MochiKit-Iter.html @@ +4,4 @@ > <script type="text/javascript" src="../MochiKit/Base.js"></script> > <script type="text/javascript" src="../MochiKit/Iter.js"></script> > <script type="text/javascript" src="../MochiKit/DOM.js"></script> > <script type="text/javascript" src="../MochiKit/Style.js"></script> same question here
Attachment #550480 - Flags: review?(jmaher) → review+
Comment on attachment 551066 [details] [diff] [review] no newlines added for any of the other half of plain tests. Review of attachment 551066 [details] [diff] [review]: ----------------------------------------------------------------- looks good!
Attachment #551066 - Flags: review?(jmaher) → review+
Comment on attachment 551067 [details] [diff] [review] no new lines in the chrome/b-c test updates Review of attachment 551067 [details] [diff] [review]: ----------------------------------------------------------------- overall, this looks good, can you just clarify the $(var) access as per my comment below? ::: content/xul/templates/tests/chrome/test_tmpl_menulistelement.xul @@ -10,5 +10,3 @@ > > onload="$('root').selectedItem = null; test_template();" > > xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > > <script type="application/javascript" > > - src="chrome://mochikit/content/MochiKit/packed.js"></script> > > - <script type="application/javascript" onload=$('root')... do we have $(var) access here with packed.js removed?
Attachment #551067 - Flags: review?(jmaher) → review+
Attachment #551068 - Flags: review?(jmaher) → review+
Comment on attachment 551061 [details] [diff] [review] No more newlines added when replacing the packed.js script tag Review of attachment 551061 [details] [diff] [review]: ----------------------------------------------------------------- thanks for the cleanup!
Attachment #551061 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #15) > It might be nice to create a mochikit_lite.js or something which has all the > function we needed to rip out and use in the harness. It seems like that > would reduce code duplication. My only concern with that is having another file in cache to load for each test. I think we can do that provided I do more profiling on this change. > in the other DOMTestCase.js we check if this is a known TODO and throw a > todo. This is true, that line is useful for knowing what the error was in the todo test. I'll add that in the next patch. > @@ +698,5 @@ > > } else { > > ok(false, "Test threw exception: " + ex); > > } > > } > > +} > > why did this change? The newline at the end of the file was removed. > ::: testing/mochitest/tests/SimpleTest/EventUtils.js > @@ +20,5 @@ > > + return ((typeof(id) == "string") ? > > + document.getElementById(id) : id); > > +}; > > + > > +this.$ = this.getElement; > > why do you do this here? I see you replace a lot of $('...') calls > elsewhere when you can do the same trick > I do this here because the most of the tests that use EventUtils.js don't use SimpleTest.js, so I moved the two packed.js functions, addLoadEvent and $(''), into this file. I replaced the $('') calls only in files that don't use SimpleTest.js or EventUtils.js, and only used packed.js for this helper (ex: testing/mochitest/harness-overlay.xul, docshell/test/bug94514-postpage.html). Most of those files only have one $ reference change, but TestRunner.js has many substitutions and I should add the helper there in the next patch. > @@ +37,5 @@ > > + this[path] = null; > > + } catch (e) { > > + // pass > > + } > > + } > > what are you doing here with 'once' ? ah, I missed that when I refactored something. Will fix in next patch. > ::: testing/mochitest/tests/SimpleTest/LogController.js > @@ +1,2 @@ > > +var LogController = {}; > > + > > can we put a license header in this file? Also a few comments. It is > mostly self explanatory. > Will do. > @@ +66,5 @@ > > + if (pair.ident != k || (pair[0] && !pair[0](msg))) > > + continue; > > + pair[1](msg); > > + } > > +}; > > this function isn't spaced the same as the rest of the file > > @@ +74,5 @@ > > + filter = LogController.logLevelAtLeast(filter); > > + } > > + else { > > + > > + } > > why the blank else{} clause? Also spacing needs to be cleaned up here Overlooked after my many code changes:( Will fix! > ::: testing/mochitest/tests/SimpleTest/SimpleTest.js > @@ +90,5 @@ > > + } > > + func(args); > > + }; > > + }; > > +} > > can we get a little comment explaining what partial is supposed to do and > who uses it? Also the condition blocks here have no whitespace between them > and the {, please make this like the functions above and below. Good idea. > @@ +153,5 @@ > > + window["onload"] = regfunc; > > + } > > + regfunc.callStack.push(func); > > + }; > > +} > > this is duplicated code in EventUtils.js? Is that necessary, or can we > minimize the duplication? Good eye, I have to clean that up. only one test, layout/base/tests/bug613807-1.html, uses this function, but I just tested this and this call can safely be replaced with a 'body onload' call instead, since this is being called within its own iframe in layout/base/tests/test_reftests_with_caret.html. > @@ +368,5 @@ > > /** > > * Toggle element visibility > > **/ > > SimpleTest.toggle = function(el) { > > + if (el.style && (el.style.display == 'block')) { > > why not use computedStyle from up above here? Good catch, I replaced a call to computedStyle early in the project and forgot to update it. > ::: testing/mochitest/tests/SimpleTest/TestRunner.js > @@ +90,5 @@ > > if (TestRunner._currentTest < TestRunner._urls.length) { > > var runtime = new Date().valueOf() - TestRunner._currentTestStartTime; > > if (runtime >= TestRunner.timeout * TestRunner._timeoutFactor) { > > + var frameWindow = document.getElementById('testframe').contentWindow.wrappedJSObject || > > + document.getElementById('testframe').contentWindow; > > why not create a function getElement() as you have in SimpleTest.js and > EventUtils.js? I will, I neglected to fix that up earlier:( Thanks for the review, and sorry for the weird use of whitespace! I'll update the patch with the relevant changes after testing.
(In reply to Joel Maher (:jmaher) from comment #20) > Comment on attachment 550480 [details] [diff] [review] [diff] [details] [review] > Update the packed.js dependent tests > ::: browser/components/places/tests/browser/browser_library_batch_delete.js > @@ +79,5 @@ > > // Test live update. > > PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId, > > makeURI(TEST_URL), > > PlacesUtils.bookmarks.DEFAULT_INDEX, > > + "bm"); > > why is this 'i' removed? > > ::: > browser/devtools/webconsole/test/browser/ > browser_webconsole_bug_601667_filter_buttons.js > @@ +91,5 @@ > > ok(isChecked(menuItem), "menu item " + prefKey + " for category " + > > aCategory + " is checked after clicking the button"); > > ok(HUDService.filterPrefs[hudId][prefKey], prefKey + " messages are " + > > "on after clicking the button"); > > + menuItem = menuItem.nextSibling; > > why is this i++ removed? > > ::: content/html/content/test/test_bug100533.html > @@ +8,5 @@ > > <script type="text/javascript" src="/MochiKit/packed.js"></script> > > + <script type="text/javascript" src="/MochiKit/Base.js"></script> > > + <script type="text/javascript" src="/MochiKit/DOM.js"></script> > > + <script type="text/javascript" src="/MochiKit/Style.js"></script> > > + <script type="text/javascript" src="/MochiKit/Signal.js"></script> > > why are you not removing packed.js, but you are for the > content/media/test/test_contentDuration*.html tests below? > > ::: content/media/test/test_delay_load.html > @@ +101,5 @@ > > // the video's finished loading (in the other document). > > // Opening a new window to do this is a bit annoying, but if we use an iframe here, > > // delaying of the iframe's load event might interfere with the firing of our load event > > // in some confusing way. So it's simpler just to open another window. > > +var w = window.open("", "testWindow", "width=400,height=400"); > > why did you remove the "+i" here? These were the instances where tests were using the i variable that was being leaked from packed.js. The i variables weren't defined within the scope of the files, and weren't actually being used: In the first file, it was left over from a copy paste of a line that was being used in a for loop that had an i variable. In the second file, I'm not sure why there was an i variable being used, but it doesn't serve any purpose, it was being incremented without being used anywhere in the file (it looks like leftover code). In the third file, the i doesn't do anything substantial again. At first it seemed the i variable was being used to establish a unique name for a newly opened window, but that isn't the case and it looks like it's another case of leftover code, since it adds no functionality. > > ::: dom/plugins/test/mochitest/test_npruntime_npnevaluate.html > @@ +78,4 @@ > > typeof(result) + "), expected " + json_expected + "(" + > > + typeof(expected) + ")")); > > + $("verbose").appendChild( > > + createEl('br') > > how do you access $('verbose') here? In the previous file, we needed to do > document.getElementById("testForm"). > The previous file (docshell/test/bug94514-postpage.html ) doesn't use SimpleTest.js, so it didn't have access to the $ helper function. > ::: dom/tests/mochitest/dom-level1-core/test_hc_attrremovechild2.html > @@ +86,5 @@ > > + if (testFails(o)) { > > + failures.push(o); > > + } > > + } > > + // shouldn't be 0 failures > > I don't understand why this specific test case is different from the other > dom-level1-core tests? We are removing markTodos() from all the tests but > this one? This is a tricky one. It seems this test actually passes, as opposed to other todo tests. That's why in its markTodos function, it uses ok(...) instead of todo(...) like the other tests. It was written to be this way, but I'm not sure why it is still marked as a Todo test. > > ::: testing/mochitest/tests/MochiKit-1.4.2/tests/test_MochiKit-Base.html > @@ +4,4 @@ > > <script type="text/javascript" src="../MochiKit/Base.js"></script> > > <script type="text/javascript" src="../MochiKit/Iter.js"></script> > > <script type="text/javascript" src="../MochiKit/DOM.js"></script> > > <script type="text/javascript" src="../MochiKit/Style.js"></script> > > why is SimpleTest.js moved to the top here? This seems unnecessary? > ::: testing/mochitest/tests/MochiKit-1.4.2/tests/test_MochiKit-Iter.html > @@ +4,4 @@ > > <script type="text/javascript" src="../MochiKit/Base.js"></script> > > <script type="text/javascript" src="../MochiKit/Iter.js"></script> > > <script type="text/javascript" src="../MochiKit/DOM.js"></script> > > <script type="text/javascript" src="../MochiKit/Style.js"></script> > > same question here These tests use and test functions from MochiKit, but still use SimpleTest. SimpleTest has some simplified versions of some of the functions in MochiKit, so if it follows the script tags, the functions in MochiKit will be overwritten by the similar functions in SimpleTest, but their behaviors are not exactly the same, and would cause errors. I will add a comment about that in the next patch.
(In reply to Joel Maher (:jmaher) from comment #22) > Comment on attachment 551067 [details] [diff] [review] [diff] [details] [review] > no new lines in the chrome/b-c test updates > > Review of attachment 551067 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > overall, this looks good, can you just clarify the $(var) access as per my > comment below? > > ::: content/xul/templates/tests/chrome/test_tmpl_menulistelement.xul > @@ -10,5 +10,3 @@ > > > onload="$('root').selectedItem = null; test_template();" > > > xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > > > <script type="application/javascript" > > > - src="chrome://mochikit/content/MochiKit/packed.js"></script> > > > - <script type="application/javascript" > > onload=$('root')... > > do we have $(var) access here with packed.js removed? Yes, the $ helper is established in SimpleTest.js, which it includes
(In reply to Joel Maher (:jmaher) from comment #20) > ::: content/html/content/test/test_bug100533.html > @@ +8,5 @@ > > <script type="text/javascript" src="/MochiKit/packed.js"></script> > > + <script type="text/javascript" src="/MochiKit/Base.js"></script> > > + <script type="text/javascript" src="/MochiKit/DOM.js"></script> > > + <script type="text/javascript" src="/MochiKit/Style.js"></script> > > + <script type="text/javascript" src="/MochiKit/Signal.js"></script> > > why are you not removing packed.js, but you are for the > content/media/test/test_contentDuration*.html tests below? Oh, I left that out, but it gets covered by the first patch that removes all the packed.js script tags.
(In reply to Malini Das from comment #25) > > ::: dom/tests/mochitest/dom-level1-core/test_hc_attrremovechild2.html > > @@ +86,5 @@ > > > + if (testFails(o)) { > > > + failures.push(o); > > > + } > > > + } > > > + // shouldn't be 0 failures > > > > I don't understand why this specific test case is different from the other > > dom-level1-core tests? We are removing markTodos() from all the tests but > > this one? > > This is a tricky one. It seems this test actually passes, as opposed to > other todo tests. That's why in its markTodos function, it uses ok(...) > instead of todo(...) like the other tests. It was written to be this way, > but I'm not sure why it is still marked as a Todo test. Nothing tricky here, smaug just enabled this test incorrectly. Please remove "hc_attrremovechild2" from dom/tests/mochitest/dom-level1-core/exclusions.js.
(In reply to Ms2ger from comment #28) > (In reply to Malini Das from comment #25) > > > ::: dom/tests/mochitest/dom-level1-core/test_hc_attrremovechild2.html > > > @@ +86,5 @@ > > > > + if (testFails(o)) { > > > > + failures.push(o); > > > > + } > > > > + } > > > > + // shouldn't be 0 failures > > > > > > I don't understand why this specific test case is different from the other > > > dom-level1-core tests? We are removing markTodos() from all the tests but > > > this one? > > > > This is a tricky one. It seems this test actually passes, as opposed to > > other todo tests. That's why in its markTodos function, it uses ok(...) > > instead of todo(...) like the other tests. It was written to be this way, > > but I'm not sure why it is still marked as a Todo test. > > Nothing tricky here, smaug just enabled this test incorrectly. Please remove > "hc_attrremovechild2" from dom/tests/mochitest/dom-level1-core/exclusions.js. Thanks for the info. I'll remove it from the excluded tests and remove its markTodos function
Changed as per review comments
Attachment #550479 - Attachment is obsolete: true
Attachment #551994 - Flags: review?(jmaher)
I moved the test accidentally marked as todo out of exclusions.js
Attachment #550480 - Attachment is obsolete: true
Attachment #551995 - Flags: review?(jmaher)
I had to update this since one of the tests has been moved
Attachment #551061 - Attachment is obsolete: true
Attachment #551996 - Flags: review?(jmaher)
One of the tests moved; had to update.
Attachment #551066 - Attachment is obsolete: true
Attachment #551997 - Flags: review?(jmaher)
the tests that were moved from the old patches are updated here, in their new paths.
Attachment #551068 - Attachment is obsolete: true
Attachment #551998 - Flags: review+
mdas, these uploaded patches are all 19 bytes 'no patches applied', can you upload the new set of patches and just ask for review on the harness changes?
Attachment #551994 - Attachment is obsolete: true
Attachment #552104 - Flags: review?(jmaher)
Attachment #551994 - Flags: review?(jmaher)
I moved the test accidentally marked as todo out of exclusions.js
Attachment #551995 - Attachment is obsolete: true
Attachment #552105 - Flags: review?(jmaher)
Attachment #551995 - Flags: review?(jmaher)
Attachment #551998 - Attachment is obsolete: true
Attachment #552109 - Flags: review+
Comment on attachment 552104 [details] [diff] [review] Remove packed.js dependencies in the harnesses Review of attachment 552104 [details] [diff] [review]: ----------------------------------------------------------------- this patch is fine except for 2 big nits: - please use 2 space indentation if the rest of the file is using it - please use a consistent whitespace method in your if/for/function syntax lines. marking r+ since the patch looks good and assuming we check it in after these nits are fixed. fwiw, I don't think I put a comment in all the places that need nit fixups. ::: dom/tests/mochitest/dom-level1-core/DOMTestCase.js @@ +678,5 @@ > + } > + // shouldn't be 0 failures > + todo(SimpleTest._tests != 0 && failures == 0, "test marked todo should fail somewhere"); > + } > +} mixing up 2 and 4 space indentation, 2 space please. @@ +683,5 @@ > + > +function runJSUnitTests() { > + try { > + var tests = exposeTestFunctionNames(); > + for(var i = 0; i < tests.length; i++){ please add whitespace in the for loop. ::: dom/tests/mochitest/dom-level1-core/exclusions.js @@ +99,5 @@ > + for(var i=1; i<arguments.length; i++){ > + f = f.concat(arguments[i]); > + } > + return f; > +} 2 space indentation please, and whitespace after the condition and argument lists. ::: dom/tests/mochitest/dom-level2-core/DOMTestCase.js @@ +678,5 @@ > + o = tests[i]; > + if (testFails(o)) { > + failures.push(o); > + } > + } mixed indentation here, the rest of the file is 2 space. @@ +690,5 @@ > try { > + var tests = exposeTestFunctionNames(); > + for(var i = 0; i < tests.length; i++){ > + window[tests[i]](); > + } mixed indentation here, also we should have whitespace between the for and the { similar to: for (var i = 0; i < tests.length; i++) { ::: dom/tests/mochitest/dom-level2-core/exclusions.js @@ +71,5 @@ > + f = f.concat(arguments[i]); > + } > + return f; > +} > + 2 space indentation please, and white space after argument list. ::: layout/base/tests/bug613807-1.html @@ +6,5 @@ > </head> > <body> > <textarea id="t" rows="4"></textarea> > <script> > + if(typeof(addLoadEvent) == 'undefined'){ whitespace needed here @@ +39,5 @@ > + } > + regfunc.callStack.push(func); > + }; > + } > + 2 space indentation please. ::: testing/mochitest/tests/SimpleTest/EventUtils.js @@ +21,5 @@ > + document.getElementById(id) : id); > +}; > + > +this.$ = this.getElement; > + 2 space indentation please ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +95,5 @@ > + }; > + }; > +} > + > +if(typeof(getElement) == 'undefined'){ can you fix the whitespace for the 17 lines above this @@ +121,5 @@ > + rval.callStack = []; > + return rval; > +}; > + > +if(typeof(addLoadEvent) == 'undefined'){ can you fix the whitespace here ::: testing/mochitest/tests/SimpleTest/setup.js @@ +80,1 @@ > 2 space indent like the rest of the file.
Attachment #552104 - Flags: review?(jmaher) → review+
Attachment #552105 - Flags: review?(jmaher) → review+
Changed as per comments
Attachment #552104 - Attachment is obsolete: true
Attachment #552441 - Flags: review?(jmaher)
Attachment #552109 - Attachment is obsolete: true
Attachment #552445 - Flags: review+
I think I've learned my whitespacing lesson.
Attachment #552441 - Attachment is obsolete: true
Attachment #552572 - Flags: review?(jmaher)
Attachment #552441 - Flags: review?(jmaher)
Comment on attachment 552572 [details] [diff] [review] Remove packed.js dependencies in the harnesses [Checked in: Comment 55] Review of attachment 552572 [details] [diff] [review]: ----------------------------------------------------------------- looks great!
Attachment #552572 - Flags: review?(jmaher) → review+
landed on m-c: http://hg.mozilla.org/mozilla-central/rev/76505e286fba
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
It looks like the goal of this bug has changed at bit since it was originally filed (i.e., it changed from having less in packed.js to removing packed.js entirely). A few things that are nice to do for those of us following along (reading hg commit logs, etc.): * when the goal of the bug changes, change the "Summary" field to reflect the change * it hg commits, please make the commit message describe what you're changing. Sometimes the bug summary isn't a very good description of this, sometimes (like in this case) it is, or rather would have been if it accurately reflected the current intent of the bug.
thanks dbaron! looking back on this, I could see how it would be confusing. My fault for poorly naming the commit messages.
Summary: Add a packed MochiKit that contains only SimpleTest dependencies → Replace the packed MochiKit dependencies in mochitest harness.
Target Milestone: --- → mozilla8
Blocks: 702172
Attachment #552573 - Attachment description: removing packed.js from new tests → removing packed.js from new tests [Checked in: Comment 48]
Depends on: 713187
Comment on attachment 552107 [details] [diff] [review] Remove packed.js script tags from mochiplain tests (second half) [Checked in: Comment 51] http://hg.mozilla.org/mozilla-central/rev/118845671081
Attachment #552107 - Attachment description: Remove packed.js script tags from mochiplain tests (second half) → Remove packed.js script tags from mochiplain tests (second half) [Checked in: Comment 51]
Comment on attachment 552106 [details] [diff] [review] Remove packed.js script tags from mochiplain tests (first half) [Checked in: Comment 52] http://hg.mozilla.org/mozilla-central/rev/95496a360748
Attachment #552106 - Attachment description: Remove packed.js script tags from mochiplain tests (first half) → Remove packed.js script tags from mochiplain tests (first half) [Checked in: Comment 52]
Comment on attachment 552105 [details] [diff] [review] Update the packed.js dependent tests [Checked in: Comment 53] http://hg.mozilla.org/mozilla-central/rev/f1c41350c548
Attachment #552105 - Attachment description: Update the packed.js dependent tests → Update the packed.js dependent tests [Checked in: Comment 53]
Comment on attachment 552443 [details] [diff] [review] Remove packed.js script tags from chrome and b-c tests [Checked in: Comment 54] http://hg.mozilla.org/mozilla-central/rev/67f6f1908b0f
Attachment #552443 - Attachment description: Remove packed.js script tags from chrome and b-c tests → Remove packed.js script tags from chrome and b-c tests [Checked in: Comment 54]
Comment on attachment 552572 [details] [diff] [review] Remove packed.js dependencies in the harnesses [Checked in: Comment 55] http://hg.mozilla.org/mozilla-central/rev/f1113713ce03
Attachment #552572 - Attachment description: Remove packed.js dependencies in the harnesses → Remove packed.js dependencies in the harnesses [Checked in: Comment 55]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: