Last modified: 2013-06-26 13:40:18 UTC
Jenkins should run GuidedTour's QUnit tests.
I'll enable this soon (after I verify the test suite works properly). Meanwhile a few (non-blocking) points based on a quick look at the file[1]: * setup/teardown for mw.config wgPageName is pointless (newMwEnvironment already does this for all of mw.config. If there's a problem, please file a bug and/or document the hack). * Testing through getParamValue $.cookie looks very suspicious. If it is indeed testing something, it certainly isn't a unit test for the logic of a particular method. Please revisit this and remove tests and/or refactor methods and their tests accordingly. Perhaps some inappropriately mixed app logic with UI. * Avoid use of assert.ok[2]. It is almost always wrong (that is to say, I've yet to first see production use of it in a good way). In case of the instanceof you might for example want to do a `assert.strictEqual( a.constructor, b, ..`. assert.ok does not give any information if the assertion is wrong, hard to debug. It also makes it easy to make a mistake, as my next point shows. * Broken use of assert.ok[3] (asserting that a literal function is "ok", which is pointless as functions are objects which cast to boolean true). This should probably assert the return value of invoking the function. Had this been compared the value to something (as opposed to "ok"), this bug would've been exposed immediately. * Suspicious global private static "gt = mw.guidedTour;". What does this achieve when put in the QUnit setup hook that is ran before each test? Note that the test suite itself appears to run fine so this won't block enabling of it via Jenkins. [1] https://github.com/wikimedia/mediawiki-extensions-GuidedTour/blob/5f16b1fcb7/tests/ext.guidedTour.lib.tests.js [2] http://www.mediawiki.org/wiki/Manual:JavaScript_unit_testing/QUnit_guidelines [3] https://github.com/wikimedia/mediawiki-extensions-GuidedTour/blob/5f16b1fcb7/tests/ext.guidedTour.lib.tests.js#L282-L305
(In reply to comment #1) > I'll enable this soon (after I verify the test suite works properly). > > Meanwhile a few (non-blocking) points based on a quick look at the file[1]: > > * setup/teardown for mw.config wgPageName is pointless > (newMwEnvironment already does this for all of mw.config. If there's a > problem, please file a bug and/or document the hack). Done. Back when I added that, IIRC it was the norm to do that. > * Testing through getParamValue $.cookie looks very suspicious. > If it is indeed testing something, it certainly isn't a unit test for the > logic > of a particular method. Please revisit this and remove tests and/or > refactor > methods and their tests accordingly. Perhaps some inappropriately mixed app > logic with UI. I don't know which test(s) you're referring to, since getParamValue is used multiple times. > * Avoid use of assert.ok[2]. It is almost always wrong (that is to say, I've > yet to first see production use of it in a good way). In case of the > instanceof > you might for example want to do a `assert.strictEqual( a.constructor, b, > ..`. instanceof walks the prototype chain, as intended (I may choose to add exception subclasses later); your example does not, so I'll just use a strict comparison to true instead. > * Broken use of assert.ok[3] (asserting that a literal function is "ok", > which is pointless as functions are objects which cast to boolean true). > This should probably assert the return value of invoking the function. Yes, good catch. I will change them to strictEqual with true. > * Suspicious global private static "gt = mw.guidedTour;". What does this > achieve when put in the QUnit setup hook that is ran before each test? It's not a global, but you're correct that it does not have to be done in the setup. It's just a convenience shortcut, and I thought that was a good place to put it in this case. Fixed in https://gerrit.wikimedia.org/r/66318
That's merged.
Will enable it and bring PHPUnit tests in it as well.
Related URL: https://gerrit.wikimedia.org/r/70617 (Gerrit Change If6dc6fcd11db1bd6e23e22993247f4c70116ea9f)
Project mediawiki/extensions/GuidedTour change 70629,2 mwext-GuidedTour-merge: SUCCESS mwext-GuidedTour-lint: SUCCESS mwext-GuidedTour-testextensions-master: SUCCESS mwext-GuidedTour-qunit: SUCCESS :-)
