I recently upstreamed a patch to make MochiKit play nicely with SVG. We still need to pull a more recent version to be able to use these improvements though.

I assume you mean "Update the copy of MochiKit used in Mochitest"?

Yes.

Which version do you want to upgrade to? { http://mochikit.com/download.html MochiKit 1.4.2 (2008-11-27) is the most current release version }

http://mxr.mozilla.org/mozilla-central/find?string=%2FMochiKit.js%24 /testing/mochitest/MochiKit/MochiKit.js "MochiKit.MochiKit 1.4" /testing/extensions/community/chrome/content/MochiKit/MochiKit.js "MochiKit.MochiKit 1.3.1 : PACKED VERSION" /dom/tests/mochitest/ajax/mochikit/MochiKit/MochiKit.js "MochiKit.MochiKit 1.4"

(In reply to comment #4) > /testing/mochitest/MochiKit/MochiKit.js > "MochiKit.MochiKit 1.4" Bug 357523 comment 37: { From Serge Gautherie (:sgautherie) 2009-07-18 08:02:16 PDT Ftr, here is the cvs history part that is not in hg: sync' from http://svn.mochikit.com/mochikit/trunk/MochiKit/ [svn latest import] was [from] "pre-v1.4 == r1182" }

(In reply to comment #5) > Bug 357523 comment 37: http://svn.mochikit.com/mochikit/trunk/tests/ The tests were not re-imported after the initial "pre-v1.4 == r1174".

Upgrade from r1174: test.css and TestRunner.js have not changed. NB: It looks like we might want to upstream patches to these 3 files from mozilla-central to (future) MochiKit v1.5...

Dv1, updated to pass on try servers: *1 typo fixed in Makefile.in. *3 updated tests: *test_MochiKit-JSAN.html was/is never run. *test_MochiKit-DragAndDrop.html was not run (at all) before. *test_MochiKit-Style.html fails some newly-added checks. I don't know whether the 2 latest tests failures are bugs in MochiKit or "Gecko": I'll file follow-up bugs, fwiw.

why do we want to upgrade it? shouldn't we add a new copy instead?

(In reply to comment #13) > why do we want to upgrade it? shouldn't we add a new copy instead? What would be the use for the obsoleted version?

We should have the old MochiKit in the test matrix somewhere, because it's still deployed on sites and we don't want to break it unawares. For use in Mochitest I'm not sure that we need both, but if Mochitest is our implicit MochiKit coverage, we should make sure we replace it with explicit coverage of that version.

(In reply to comment #15) > We should have the old MochiKit in the test matrix somewhere, MochiKit has 13 tagged versions from 0.50 to 1.4.2: http://svn.mochikit.com/mochikit/tags/ Which version(s) do you want to explicitly test? *1.4.2: This one, obviously, as we will use it in our own test harness. *1.4 (or 1.4.1): 1.4.2 should be enough. *("pre-v1.4 == r1174" or) "pre-v1.4 == r1182": Even if we were/are using it, we shouldn't care (anymore) for such a dev rev. *1.3.1: If you want to care for an older version, this should be the one. *Any even older versions? > because it's > still deployed on sites and we don't want to break it unawares. You mean sites "in the wild, not Mozilla related", right? > For use in Mochitest I'm not sure that we need both, We obviously don't. > but if Mochitest is our implicit MochiKit coverage, I wasn't aware of this goal, > we should make sure we replace it with explicit coverage of that version. but that can make sense, and is trivial to do.

Ftr, test_MochiKit-DOM-Safari.html, test_MochiKit-DragAndDrop.html and test_MochiKit-JSAN.html are not run by MochiKit index.html. test_MochiKit-JSAN.html is actually broken, but test_MochiKit-DOM-Safari.html passes as is. test_MochiKit-DragAndDrop.html initially failed with "not ok - test suite failure! message: MochiKit.DragAndDrop.Draggable is not a constructor fileName: http://localhost:8888/tests/MochiKit-1.4.2/tests/test_DragAndDrop.js lineNumber: 7 stack: [...]" but the Error Console reported "Error: uncaught exception: MochiKit.Visual depends on MochiKit.Position!" and "Error: uncaught exception: MochiKit.DragAndDrop depends on MochiKit.Position!" so I added the missing script "include" to make the test pass. And I added a workaround to test_MochiKit-Style.html: see comment 12...

Fv1 + Gv1 passed on m-c try.

Comment on attachment 390141 [details] [diff] [review] (Fv1) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), add bare files [Checkin: See comment 39 & 46 & 49] Ping for reviews of Fv1 and Gv1.

these are huge and will require considerable time to review and test. Time I don't have at the moment. I'll get back at this next week unless someone beats me to it. serge, these try builds passed on all platforms? Can you point to the logs?

pressed "commit" while still reading the timestamp on your comment #19. That was a month ago, so finding those logs could be difficult.

(In reply to comment #21) > these are huge and will require considerable time to review and test. Fv1 is 555 kB but it's only importing files from MochiKit svn: no code to review actually, just looking for an answer/confirmation to comment 16 question/list about versions ;-) Gv1 is 31 kB but it's mostly makefiles and repeated same path fixes: the "additional" changes to review are very few and easy (see comment 18 (and comment 12)) and touch tests only (not MochiKit code itself). Fwiw, my plan is: 1- Land these. (and do the same for other versions if wanted.) 2- Rework the other patches. 3- Upstream whatever fixes which were not yet.

I'm not going to just put a rubber stamp on these things without verifying for myself that it doesn't break our unittest suite. This means a full build and unittest run. I also expect there may be landing difficulties with this on our production unittest builders. It'll have to wait until I have time to review it properly.

(In reply to comment #24) > I'm not going to just put a rubber stamp on these things without verifying for > myself that it doesn't break our unittest suite. This means a full build and > unittest run. Comment 19 was meant to cover this... That said, I obviously see no problem in having you check for yourself. > I also expect there may be landing difficulties with this on our > production unittest builders. It'll have to wait until I have time to review it > properly. Well, if tests passed on Try and would then fail on Production, then there would be something really wrong in the process... Would it be enough if I re-submit these patches to Try? Or do you plan on doing some different testing actions yourself?

Ping for review, anyone?

Sorry I don't have time to review these behemoths right now. Also, landing significant changes to our test suites could be dangerous given the proximity to 1.9.2 release.

Comment on attachment 390141 [details] [diff] [review] (Fv1) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), add bare files [Checkin: See comment 39 & 46 & 49] 9 months later: still pinging for review!

alright, are all of these patches still valid?

(In reply to comment #29) > alright, are all of these patches still valid? Patches F and G are: see comment 23.

So, I ran the F and G patches through try server last night and these tests seemed to pass. Here are the logs: * Linux: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271118992.1271129436.509.gz * Mac: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271118992.1271132262.8381.gz * Windows: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271118991.1271132135.8036.gz I also did some diff's between this and an unpatched build and I have some questions to be sure I understand what you're doing here, Serge. It looks like you're just adding in the new Mochitest so that it becomes <objdir>/_tests/testing/mochitest/tests/MochiKit-1.4.2 and is included for testing in our grand pass of mochitests. You're not changing the default MochiKit installation that the infrastructure uses. And I should note that this does take down the "scary" factor of these two patches quite a bit as this really only adds another test suite to mochitest. It doesn't actually affect the current mochitest harness infrastructure at all. Is that what you were trying to get across in comment 23? I might be block-headed here, but allow me to outline this as I see it: * Patch F & G get us the updated mochikit code in the tree so that we can run it and run the MochiKit unit tests on that code. * Patch A & B actually put the updated 1.4.2 code into the test harness and change the automation to use the updated code from that point forward. * You want us to review F & G first, and then we'll work on changing the harness, right? Is that all correct? (My apologies if that was clear to everyone else).

(In reply to comment #31) > So, I ran the F and G patches through try server last night and these tests > seemed to pass. Thanks. (This confirms comment 19 :-)) > It looks like you're just adding in the new Mochitest so that it becomes > <objdir>/_tests/testing/mochitest/tests/MochiKit-1.4.2 and is included for > testing in our grand pass of mochitests. You're not changing the default > MochiKit installation that the infrastructure uses. Correct: that's the comment 16 part. I'm leaving out tests/index.html and tests/SimpleTest/*, because we want to check their code and tests but inside our (modified) harness. > Is that what you were trying to get across in comment 23? Exactly. > * Patch F & G get us the updated mochikit code in the tree so that we can run > it and run the MochiKit unit tests on that code. Yes: at this point, the code is included for the only purpose of running the tests on it. > * Patch A & B actually put the updated 1.4.2 code into the test harness and > change the automation to use the updated code from that point forward. Yes: the main point is I am/will splitting out the 2 currently-mixed purposes: *Testing (bare) public MochiKit itself. *Using (a tweaked version of) it to run our own mochitests. > * You want us to review F & G first, and then we'll work on changing the > harness, right? Yes: I'll see what's left of A to E patches at that point.

Comment on attachment 390141 [details] [diff] [review] (Fv1) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), add bare files [Checkin: See comment 39 & 46 & 49] I ran through the diffs between this and the 1.4 version we currently have in use on m-c. There are some things in here that will likely break some tests - mochiKit is capturing scroll events now and mousewheel events, and I pessimisticly believe that will cause test failures once we turn this on as the default MochiKit base for the harness. That said, since this patch only adds the new MochiKit 1.4.2 version as a test suite inside mochitest/tests/MochiKit-1.4.2/MochiKit it is safe to take now and we can deal with the fallout of switching to 1.4.2 as the default later on. r+ for landing as another test harness inside mochitest.

Comment on attachment 390161 [details] [diff] [review] (Gv1) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), make it run and pass This is ok, but I have a couple of things that ought to be changed and one question. >+++ b/testing/mochitest/tests/MochiKit-1.4.2/Makefile.in These comments apply to the next couple of license blocks in the patch. >+# >+# ***** BEGIN LICENSE BLOCK ***** >+# Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+# >+# The contents of this file are subject to the Mozilla Public License Version >+# 1.1 (the "License"); you may not use this file except in compliance with >+# the License. You may obtain a copy of the License at >+# http://www.mozilla.org/MPL/ >+# >+# Software distributed under the License is distributed on an "AS IS" basis, >+# WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+# for the specific language governing rights and limitations under the >+# License. >+# >+# The Original Code is MochiKit code. The original code in this case refers to this Makefile.in itself. It does not refer to the MochiKit code (as I understand it). So, the initial developer is you or your employer as the case may be. I know that the code that you're wrapping with the makefile is indeed MochiKit code, but the MPL is a per-file license and only refers to our current file, which is this makefile which is your code and not MochiKit's. We have included their license in patch F, which I think is all we have to do here. Now, if this is actually a makefile that mochiKit used in their codebase, then you should leave it as is, but I doubt that MochiKit uses our style of makefiles. >+# >+# The Initial Developer of the Original Code is Bob Ippolito. Likewise, the initial developer of this makefile is you, not Mr. Ippolito. >+# Portions created by the Initial Developer are Copyright (C) 2005 And you developed the makefile in 2010, not 2005. ^ Apply those nits to the next two makefiles in the patch too, please. I actually went and looked this up cause I wasn't sure. It is covered under point 4 on this page: http://www.mozilla.org/MPL/license-policy.html > >@@ -164,34 +164,37 @@ try { > is(dims.w > 0, true, 'test getViewportDimensions w'); > is(dims.h > 0, true, 'test getViewportDimensions h'); > > pos = getViewportPosition(); > is(pos.x, 0, 'test getViewportPosition x'); > is(pos.y, 0, 'test getViewportPosition y'); > > dims = getElementDimensions('testCell1', true); >- is(dims.w, 80, 'default left table cell content w ok'); >+ // Conditionally disable 6 'is(dims.w, 80, ...)' checks, >+ // which fail when test is not run standalone. >+ var testFn = dims.w == 80 ? is : todo_is; Do we know why this fails when it is not run standalone? Is it just because that's how the MochiKit folks intended the test to run? Or is something actually broken here? If it's just how the mochiKit folks intended the test to run, then I'm ok with this change. If something is broken, then let's file a follow-up bug. >+ testFn(dims.w, 80, 'default left table cell content w ok'); > is(dims.h, 30, 'default left table cell content h ok'); > dims = getElementDimensions('testCell2', true); >- is(dims.w, 80, 'default middle table cell content w ok'); >+ testFn(dims.w, 80, 'default middle table cell content w ok'); > is(dims.h, 30, 'default middle table cell content h ok'); > dims = getElementDimensions('testCell3', true); >- is(dims.w, 80, 'default right table cell content w ok'); >+ testFn(dims.w, 80, 'default right table cell content w ok'); > is(dims.h, 30, 'default right table cell content h ok'); > > setStyle('testTable', {'borderCollapse': 'collapse'}); > dims = getElementDimensions('testCell1', true); >- is(dims.w, 80, 'collapsed left table cell content w ok'); >+ testFn(dims.w, 80, 'collapsed left table cell content w ok'); > is(dims.h, 30, 'collapsed left table cell content h ok'); > dims = getElementDimensions('testCell2', true); >- is(dims.w, 80, 'collapsed middle table cell content w ok'); >+ testFn(dims.w, 80, 'collapsed middle table cell content w ok'); > is(dims.h, 30, 'collapsed middle table cell content h ok'); > dims = getElementDimensions('testCell3', true); >- is(dims.w, 80, 'collapsed right table cell content w ok'); >+ testFn(dims.w, 80, 'collapsed right table cell content w ok'); > is(dims.h, 30, 'collapsed right table cell content h ok'); > > hideElement('testTable'); > > var overflow = makeClipping('styleTest'); > is(getStyle('styleTest', 'overflow-x'), 'hidden', 'make clipping on overflow-x'); > is(getStyle('styleTest', 'overflow-y'), 'hidden', 'make clipping on overflow-y'); > r=me with those nits addressed.

Comment on attachment 390141 [details] [diff] [review] (Fv1) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), add bare files [Checkin: See comment 39 & 46 & 49] I'm going to check this patch in without the following files which are useless to run these tests as mochitests: MochiKit/__package__.js tests/FakeJSAN.js tests/cli.js tests/standalone.js Can you confirm your review?

Gv1, with comment 34 suggestion(s): *License: good catch! At that time, I didn't know much about what should be done. *test_MochiKit-Style.html: I eventually looked deeper and "fixed" it ;-> plus *The biggest improvement: I'm now using local SimpleTest stubs :-)

(In reply to comment #35) > (From update of attachment 390141 [details] [diff] [review]) > > I'm going to check this patch in without the following files which are useless > to run these tests as mochitests: > MochiKit/__package__.js > tests/FakeJSAN.js > tests/cli.js > tests/standalone.js > > Can you confirm your review? This looks good. I wonder if we shouldn't also include test_MochiKit-JSAN.html in that list too since it can't be run without the JSAN and since you note that it will be removed in mochikit 1.5 anyway. My only nit is that all these files are still in this updated patch, so please take care not to add them when you go to check this in.

(In reply to comment #36) > Created an attachment (id=439092) [details] > (Gv1a) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ > (== r1480), make it run and pass > > Gv1, with comment 34 suggestion(s): > *License: good catch! At that time, I didn't know much about what should be > done. > *test_MochiKit-Style.html: I eventually looked deeper and "fixed" it ;-> > plus > *The biggest improvement: I'm now using local SimpleTest stubs :-) That's awesome. I like the stub fix. :) r=me

Comment on attachment 390141 [details] [diff] [review] (Fv1) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), add bare files [Checkin: See comment 39 & 46 & 49] http://hg.mozilla.org/mozilla-central/rev/3e10611826dc Fv1, updated per my comment 35. (In reply to comment #37) > I wonder if we shouldn't also include test_MochiKit-JSAN.html in that list I excluded unneeded support files, but I prefer to import all code and tests: ftr. > My only nit is that all these files are still in this updated patch Because I did not attach the updated (550 kB) patch ;->

Comment on attachment 439092 [details] [diff] [review] (Gv1a) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), make it run and pass [Checkin: Comment 40 & 47 & 50] http://hg.mozilla.org/mozilla-central/rev/89ab9d52bef2

Remove older tests now that we run newer ones. NB: We made a few fixes to MochiKit (patch Bv1) but an error in such a fix would break some actual tests in the tree.

Clint, ping for review.

(In reply to comment #42) > Clint, ping for review. Sorry Serge, will get to it today.

Comment on attachment 439600 [details] [diff] [review] (Dv2) Remove MochiKit r1174 tests [Checkin: Comment 45 & 48 & 51] Looks fine. Sorry about letting the review fall through the cracks on that one.

Comment on attachment 439600 [details] [diff] [review] (Dv2) Remove MochiKit r1174 tests [Checkin: Comment 45 & 48 & 51] http://hg.mozilla.org/mozilla-central/rev/0efb08f1b29a

Comment on attachment 390141 [details] [diff] [review] (Fv1) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), add bare files [Checkin: See comment 39 & 46 & 49] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/aed8c99f6b51

Comment on attachment 439092 [details] [diff] [review] (Gv1a) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), make it run and pass [Checkin: Comment 40 & 47 & 50] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6634c1a1cb7a

Comment on attachment 439600 [details] [diff] [review] (Dv2) Remove MochiKit r1174 tests [Checkin: Comment 45 & 48 & 51] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/82029072178f

Comment on attachment 390141 [details] [diff] [review] (Fv1) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), add bare files [Checkin: See comment 39 & 46 & 49] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4277196b52c7

Comment on attachment 439092 [details] [diff] [review] (Gv1a) Explicitly test http://svn.mochikit.com/mochikit/tags/MochiKit-1.4.2/ (== r1480), make it run and pass [Checkin: Comment 40 & 47 & 50] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ec2e3420b181

Comment on attachment 439600 [details] [diff] [review] (Dv2) Remove MochiKit r1174 tests [Checkin: Comment 45 & 48 & 51] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ca30a1bdf60b

Mass closing mochitest bugs that haven't had activity in the past 5 years. Please re-open or file a new bug with modern context if this is still relevant.