"Krinkle" posted a comment on MediaWiki.r109677.
URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/109677#c29836
Commit summary for MediaWiki.r109677:
[WebFonts.tests] Fixes
* Variables isFontFaceLoaded and fontFamilyList are lacking a "var" statement,
were leaking to global scope. Adding local closure and turning them into
(local) function expressions and moved to the top of the scope for clarity.
* Removing redundant tests:
** ok( $inputElement.remove() )
** ok( $( 'body' ).append( .. ) )
-- jQuery collections always return a jQuery object, they are chainable
prototypes without a status-related return value. These checks don't do
anything, they will always return a jQuery object and all objects case to true
in JavaScript
* Removing useless tests:
** assertTrue( $selectElement !== [] )
** In JavaScript all objects are compared by reference, this comparison can
never succeed because 1) jQuery objects are not arrays, 2) even the, "[] !==
[]" because they are both separate instances of Array. Arrays and objects must
be iterated in order to compare them by value, this is done by deepEqual.
* Fixing element selection.
-- Instead of:
$('body').append( '<input class="foo"/>' );
var $foo = $('input.foo');
-- Use:
var $foo = $( '<input class="foo"/>' );
$('body').append( $foo );
-- No need to query the DOM for this.
* Using equal() instead of ok() as much as possible. ok() is not useful in most
unit testing because it makes you have to do the comparison inline. Which means
the value given to ok() is only the end result, not the expected/result values
themselves. Which means in case of a failure, ok() can only say "Passed" or
"Failed", whereas equal() (having both values), can output useful debug
information. This is especially important when working with test reports
generated externally (eg. through TestSwarm) in which are the test report is
all you have.
// Not very useful, the comparison is done inline and ok()
// can only know about the result of the comparison
var x = 'foo';
ok( x === 'bar' ); // Failed.
ok( x === 'foo' ); // Passed.
// Useful! The unit test is give both values so it can also mention
// both in the output:
var x = 'foo';
equal( x, 'bar' ); // Failed. Expected: "bar". Result: "foo"
equal( x, 'foo' ); // Passed. Expected: "foo".
Krinkle's comment:
Forgot to include in commit message:
* Didn't introduce an alternative for the tests of the element appending.
jQuery element creation and appending can be taken for granted at this level.
If <code><nowiki>$foo.append( '<input/>' );</nowiki></code> has to be tested,
there are way bigger problems to be concerned about.
* Fixed loose string "<code>input content</code>" that was left inside the
{{tag|body|o}}. The following line fixed it:
<pre>
- ok( $( 'body' ).append( "<input class='webfonts-testing-element'>input
content</input>"), 'An input element for testing was appended to body' );
- var $inputElement = $( 'input.webfonts-testing-element' );
+ var $inputElement = $( '<input value="input content"/>' );
</pre>
Because this:
<pre>
$("<input class='webfonts-testing-element'>input content</input>");
</pre>
actually creates '''2''' nodes, not one. Since {{tag|input|o}} is a shorttag
and cannot hold any content (only attributes), the browser's native parsers
turns that into 2 nodes:<br/><code><nowiki><input
class="webfonts-testing-element" /></nowiki></code> and a Textnode:
<code>"input content"</code>. And in older version of IE6 invalid HTML like
this can also cause an exception to be thrown, when fed to the innerHTML parser.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
