r89082 MediaWiki - Code Review archive

deepEqual(cond, true, ...) seems a lot less legible than ok(cond, ...); why change from the clear form to a less clear form?

because ok() is casting to a boolean (ie. "Hello world", {} and 42) are also "ok".

deepEqual is a strict comparison to (in this case) boolean true.

Sounds like you're trying to combine two tests here:

1. the expression returns the expected boolean result (something evaluating to true)
2. the expression's return value is always a boolean-typed value

For #1 there, ok() is both sufficient and correct.

For #2, you'd also want to test a range of cases to actually cover it correctly. But honestly there's not much need to be doing that test on language constructs like the <= and in operators, is there?

I see what you mean with combining tests but I don't think it's much of an issue here.

By the way, I didn't know you were referring to the <= and in expressions. There are also tests for mw.Map / "conf" like deepEqual( conf.exists( ... ), true, ..) and deepEqual( conf.set( ..., .... ), true, ..). I thought you were (also) referring to those. Because those indeed need to be made sure to 1) return boolean and, 2) return true/false per expectation.

I think I've covered the range of cases fairly well here by trying to throw different things at it (see here).

The <= and in.. yeah, those won't need much more than simple checks. But I like to use deepEqual (alias same), because those trigger an "Expected", "Results", "Diff" report when they don't match.

ie.:

"Expected: true"
"Results: undefined"

Whereas ok() is really basic and just reports that it wasn't OK.

deepEqual (hence it's name) aside from being a strict comparer, which is faster than loose comparison in JavaScript, in contrary to PHP) also does deep comparisons in case of objects and arrays.

For example

var b = { foo: 1 };
var c = { foo: 1 };

Neither loose or strict comparison will return true between these, because all objects are references in JS. deepEqual, in such case, does a resursive comparison of the members so that objects can be tested as well. Which makes it possible to easily test the object returns from functions like conf.get (mw.Map.prototype.get).

That's kinda summed up (strict, expected/results output, faster than loose, recursiveness) why I tend to use deepEqual by default instead of ok

-- Krinkle

Sure for cases where you really are checking the returned data type yes that makes sense -- and distinguishing a false from failed-to-return-value is valuable. It's the numerous cases of simple boolean comparisons with only two possible values getting deep-compared against 'true' that seem very strange to me. :)

"OK" vs "not OK" is exactly what we probably want for simple boolean conditions like this; there are no other possible values and seeing that we got 'false' instead of 'not OK' adds no information; it should still be showing the actual text comment that explains the purpose of the comparison, right?

I agree. For the <= and in assertions a simple ok will do fine and the description/comment is displayed at all times.

So for an example rather than this:

deepEqual( conf.exists( 'foo' ), true, 'Map.exists() returns boolean true if a key exists' );
deepEqual( conf.exists( 'notExist' ), false, 'Map.exists() returns boolean false if a key does not exists' );
deepEqual( conf.get() === conf.values, true, 'Map.get() returns the entire values object by reference (if called without arguments)' );

I'd consider this:

deepEqual( conf.exists( 'foo' ), true, 'Map.exists() returns boolean true if a key exists' );
deepEqual( conf.exists( 'notExist' ), false, 'Map.exists() returns boolean false if a key does not exists' );
ok( conf.get() === conf.values, 'Map.get() returns the entire values object by reference (if called without arguments)' );

Comparing a variable that should be true or false against the expected value directly sounds perfect, and the comments already clarify that the return type is something being checked on the conf.exists() calls.

When we're just checking the return value of an === comparison, there's no such need; using ok() lets a maintainer see here you're confirming that a condition is as expected.

You might also be able to use strictEqual() here and have it report the differing data if it ever returns the wrong thing, but the docs are a bit vague on what's actually a same-object check behind the scenes.

I didn't know about strictEqual, it's basically what I was (ab)using deepEqual(foo,bar) for and looks cleaner than ok( foo === bar). Thanks.

I've given all test suites a review and fixed all of this mess to use the appropiate functions.

See r89845 commit message and diff. Removing "todo".

deepEqual( conf.get() === conf.values, true, is wrong in so many ways.

• Comparing the result of a comparison
• Wrong use of deepEqual (should be strictEqual)

This was also fixed r89845.