https://bugzilla.wikimedia.org/show_bug.cgi?id=49032
--- Comment #2 from Matthew Flaschen <[email protected]> --- (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 -- You are receiving this mail because: You are the assignee for the bug. You are on the CC list for the bug. _______________________________________________ Wikibugs-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikibugs-l
