873126 - Introduce a common JS Assert library

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Mike de Boer [:mikedeboer]]

Introducing Assert.js, a small library that is an implementation of the CommonJS Unit Testing specification (see http://wiki.commonjs.org/wiki/Unit_Testing/1.0) for use within test harnesses we use - Mochitest and XPCShell.

Please watch this short screencast - http://www.screenr.com/CkI7 (4m34s) - to get a feel for what this looks like in practice.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Add Assert.jsm, unit tests and xpcshell/ mochitest integration

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 750515 [details] [diff] [review]
Add Assert.jsm, unit tests and xpcshell/ mochitest integration

Review of attachment 750515 [details] [diff] [review]:
-----------------------------------------------------------------

I generally like where this is going. I'd like to see a bit looser coupling between Assert.jsm and the rest of the world, if possible.

Can we split this up into multiple parts? I'd at least like to isolate the Assert.jsm work from the integration into existing systems.

::: testing/modules/Assert.jsm
@@ +13,5 @@
> +let pSlice = Array.prototype.slice;
> +
> +// 1. The assert module provides functions that throw
> +// AssertionError's when particular conditions are not met. The
> +// assert module must conform to the following interface.

Which "following interface?" Did you forget the docs?

@@ +40,5 @@
> +  } else if (typeof ctx.SimpleTest == "object") {
> +    harness = "mochitest";
> +    reporter = rep;
> +  }
> +};

This scares me a bit. What you've done is turned this module into a singleton and limited how it can be used. That *might* be OK. But, it may bite us down the road. I understand why you are doing this: you want the global "Assert" symbol to "do the right thing [depending on the execution context]." Have you thought about requiring the test harnesses to obtain an Assert instance and configure it with their specific execution needs? That would allow simultaneous uses of separate Assert instances with different semantics and wouldn't constrain us to specific usage patterns. It should also remove the coupling between this file and test harness specific execution contexts.

@@ +43,5 @@
> +  }
> +};
> +
> +// 2. The AssertionError is defined in assert.
> +// new assert.AssertionError({ message: message,

Nit: cuddle braces {message: message,

@@ +87,5 @@
> +  }
> +}
> +
> +function getMessage(self) {
> +  return truncate(JSON.stringify(self.actual, replacer), 128) + " " +

Where did the 128 magic number come from? Perhaps you should make it a constant?

@@ +116,5 @@
> +  } else if (harness == "mochitest") {
> +    context.currentTest.addResult(new reporter(false, err.message, false, false, err.stack));
> +  } else {
> +    throw err;
> +  }

I don't like the coupling here. The logic for the test harnesses should live in the test harnesses, not in Assert.jsm.

@@ +123,5 @@
> +function success(message) {
> +  if (harness == "xpcshell") {
> +    context.do_report_result(true, message);
> +  } else if (harness == "mochitest") {
> +    context.currentTest.addResult(new reporter(true, message));

It might be useful to pass more detailed information down into the success case. This would enable richer logging and metadata capture, for example. This could potentially be deferred to a followup.

@@ +137,5 @@
> +// This statement is equivalent to assert.equal(true, !!guard,
> +// message_opt);. To test strictly for the value true, use
> +// assert.strictEqual(true, guard, message_opt);.
> +function ok(value, message) {
> +  if (!!!value) {

Is there ever a case where !value != !!!value?

@@ +176,5 @@
> +    success(message);
> +  }
> +};
> +
> +function _deepEqual(actual, expected) {

Did you copy/borrow this logic from anywhere or did you roll it yourself? I've seen a few implementations of this code around the tree and I wonder if it's worth separating into its on jsm in toolkit/modules.

@@ +209,5 @@
> +  }
> +}
> +
> +function instanceOf(object, type) {
> +  return Object.prototype.toString.call(object) == "[object " + type + "]";

Why can't you use the instanceof operator?

@@ +222,5 @@
> +}
> +
> +function objEquiv(a, b) {
> +  if (isUndefinedOrNull(a) || isUndefinedOrNull(b))
> +    return false;

Nit: braces

@@ +255,5 @@
> +  //~~~cheap key test
> +  for (i = ka.length - 1; i >= 0; i--) {
> +    if (ka[i] != kb[i])
> +      return false;
> +  }

I'm not sure if this optimization is worth it. In the expected case, the objects will be equal, which means you'll have to perform all the comparisons. This may actually make comparison slower!

@@ +256,5 @@
> +  for (i = ka.length - 1; i >= 0; i--) {
> +    if (ka[i] != kb[i])
> +      return false;
> +  }
> +  // Equivalent values for every corresponding key, and possibly expensive deep 

Nit: trailing whitespace

@@ +258,5 @@
> +      return false;
> +  }
> +  // Equivalent values for every corresponding key, and possibly expensive deep 
> +  // test
> +  for (i = ka.length - 1; i >= 0; i--) {

for (let key of ka) {

@@ +260,5 @@
> +  // Equivalent values for every corresponding key, and possibly expensive deep 
> +  // test
> +  for (i = ka.length - 1; i >= 0; i--) {
> +    key = ka[i];
> +    if (!_deepEqual(a[key], b[key])) return false;

Nit: Use braces.

@@ +295,5 @@
> +    success(message);
> +  }
> +};
> +
> +function expectedException(actual, expected) {

It might be useful to compare details of the exception as part of the test. e.g. ensure a substring is contained in actual.message.

@@ +300,5 @@
> +  if (!actual || !expected) {
> +    return false;
> +  }
> +
> +  if (Object.prototype.toString.call(expected) == "[object RegExp]") {

Does |expected instanceof RegExp| not work?

@@ +346,5 @@
> +}
> +
> +// 11. Expected to throw an error:
> +// assert.throws(block, Error_opt, message_opt);
> +assert.throws = function(block, /*optional*/error, /*optional*/message) {

I've never seen /*optional*/ used in Firefox JS. Perhaps you could use the JavaDoc style comment blocks that are more or less standard.

/**
 * Expect an exception to be thrown by a function call.
 *
 * @param block
 *        (function) Function to call that should throw an exception.
 * @param error (optional)
 *        (object) Type of exception that is expected to be thrown.
 * @param message (optional)
 *        (string) Message to print on failure.
 */

@@ +347,5 @@
> +
> +// 11. Expected to throw an error:
> +// assert.throws(block, Error_opt, message_opt);
> +assert.throws = function(block, /*optional*/error, /*optional*/message) {
> +  _throws.apply(this, [true].concat(pSlice.call(arguments)));

I /think/ usage of 'arguments' is now frowned upon in Gecko JS. Rest parameters (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/rest_parameters) are the new hotness.

@@ +352,5 @@
> +};
> +
> +// EXTENSION! This is annoying to write outside this module.
> +assert.doesNotThrow = function(block, /*optional*/message) {
> +  _throws.apply(this, [false].concat(pSlice.call(arguments)));

What's the value of this? Don't test harnesses typically trap uncaught exceptions? Is this for use by the test harness itself (as opposed to individual test functions)?

::: testing/modules/moz.build
@@ +3,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +TEST_DIRS += ['tests']

bit rotted.

::: testing/modules/tests/Makefile.in
@@ +9,5 @@
> +relativesrcdir = @relativesrcdir@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +include $(topsrcdir)/config/rules.mk

You don't need to have a Makefile.in unless there are non-boilerplate statements. This Makefile.in is all boilerplate. Remove this file.

::: testing/modules/tests/browser/test_assert.js
@@ +1,1 @@
> +function test() {

Need license header.

::: testing/modules/tests/xpcshell/test_assert.js
@@ +16,5 @@
> +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
> +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
> +// USE OR OTHER DEALINGS IN THE SOFTWARE.

Oh, where did this come from? This looks like a MPL-friendly license, but I'm not an attorney. We should have Gerv review it.

This also has me asking "did any other code in this patch come from an external source?"

::: testing/xpcshell/head.js
@@ +575,5 @@
>    _do_check_neq(left, right, stack, true);
>  }
>  
>  function do_report_result(passed, text, stack, todo) {
> +  if (!stack)

Nit: always use braces

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: introduce Assert.jsm, including raw xpcshell test

`Assert` is not a singleton anymore, which makes integration into various test frameworks a lot easier (see patches 2 & 3).
Included documentation for all member functions.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: XPCShell-test integration

Assert.jsm functions are now a first-class citizen inside XPCShell-tests, which I think is nice - but up for discussion!

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: XPCShell-test integration

forgot to remove something

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: Mochitest integration - WIP

This is VERY much a work in progress (it won't work in its current state), but I'm putting it up here, because I need a working mochitest test to do TDD here.

Greg, I *think* I'm telling the build system to register the mochitest file (test_assert.js) correctly, but it tells me it can't find it! What am I doing wrong?

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 770769 [details] [diff] [review]
Patch 1: introduce Assert.jsm, including raw xpcshell test

Review of attachment 770769 [details] [diff] [review]:
-----------------------------------------------------------------

This patch makes me so happy. A unified assertion library in the tree... finally!

I have a few nits and concerns. The biggest one is the licensing. Seeing a copyright notice without a corresponding license block is giving off some bad vibes.

As far as the code goes, I think this is good enough to check in. I would encourage you to track down additional reviewers for some of the low-level JS stuff, especially deep equals. There's enough going on there that I wouldn't be surprised I missed something or if my understanding of JS equality and object definitions was lacking. Whatever the code ends up being, you'll be able to command a very good interview question!

::: testing/modules/Assert.jsm
@@ +6,5 @@
> +// When you see a javadoc comment that contains a number, it's a reference to a
> +// specific section of the CommonJS spec.
> +//
> +// Originally from narwhal.js (http://narwhaljs.org)
> +// Copyright (c) 2009 Thomas Robinson <280north.com>

Which parts are from narhwal? We may need to consult Gerv to ensure licensing is sane. Please write up a description of which parts of the code you didn't author yourself and needinfo? :gerv to seek his blessing that we're doing everything right.

@@ +156,5 @@
> +   *        (mixed) Test reference to evaluate against `actual`
> +   * @param message (optional)
> +   *        (string) Short explanation of the expected result
> +   */
> +  this.equal = function equal(actual, expected, message) {

Nit: No need to name function here.

@@ +267,5 @@
> +    // Equivalent values for every corresponding key, and possibly expensive deep 
> +    // test
> +    for (i = ka.length - 1; i >= 0; i--) {
> +      key = ka[i];
> +      if (!_deepEqual(a[key], b[key])) {

I feel like you are missing a key name compare somewhere in here. I /think/ some undefined values on an object might result in false positives for equality under the right circumstances.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 770772 [details] [diff] [review]
Patch 2: XPCShell-test integration

Review of attachment 770772 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to have a quick discussion on namespace pollution before granting r+. Otherwise, this patch looks good.

::: testing/xpcshell/head.js
@@ +358,5 @@
> +      let stack = Components.stack;
> +      // Unwind the stack until we reach the actual assert statement.
> +      do {
> +        stack = stack.caller;
> +      } while(stack.filename && stack.filename.indexOf("Assert.jsm") > -1)

Didn't I see this code in Assert.jsm? Perhaps you should just call the function in there? Remember, Cu.import() returns a "back stage pass" that allows you to access unexported symbols.

@@ +364,5 @@
> +    }
> +  };
> +  // Tack Assert.jsm methods to the current scope.
> +  for (let func in assertImpl) {
> +    this[func] = assertImpl[func].bind(assertImpl);

So, we have a decision to make: do we export each function separately into the global namespace or do we export a single symbol (Assert)?

I like not having to type "Assert." all the time. On the other hand, I really don't care for namespace pollution. I'm already quite frustrated that the namespace of xpcshell tests is as polluted as it is. I've frequently had symbol collisions, etc.

How about a compromise.

Let's export "Assert" and a utility function "export_assertions" (or similar). If tests so want, they can |export_assertions();|, which will define all the Assert.* symbols on the global this.

Thoughts?

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 770776 [details] [diff] [review]
Patch 3: Mochitest integration - WIP

Review of attachment 770776 [details] [diff] [review]:
-----------------------------------------------------------------

I guess the approach looks good. Although, you should get review from a mochitest harness expert. Who that would be, I don't know - check version control history.

::: testing/mochitest/browser-test.js
@@ +392,5 @@
>      this.currentTest.scope.SimpleTest = this.SimpleTest;
>      this.currentTest.scope.gTestPath = this.currentTest.path;
>  
> +    let assertImpl = new Assert();
> +    // Override the report function for xpcshell-test style reporting.

xpcshell?

::: testing/mochitest/jar.mn
@@ +27,5 @@
>    content/tests/SimpleTest/MockObjects.js (tests/SimpleTest/MockObjects.js)
>    content/tests/SimpleTest/NativeKeyCodes.js (tests/SimpleTest/NativeKeyCodes.js)
>    content/tests/SimpleTest/paint_listener.js (tests/SimpleTest/paint_listener.js)
>    content/tests/SimpleTest/docshell_helpers.js (../../docshell/test/chrome/docshell_helpers.js)
> +  content/tests/SimpleTest/Assert.jsm (../modules/Assert.jsm)

I don't believe you need to do this. I /thought/ we packaged testing-only JSMs and made them available to mochitest tests. I could be wrong (in which case there is an open bug somewhere we should fix).

::: testing/modules/tests/browser/Makefile.in
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + 

Nit: trailing whitespace (few other parts in file as well).

::: testing/modules/tests/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +DIRS += ['browser']
> +
> +MODULE = 'test_testing_general'

Is MODULE actually required here? I don't think so. (But I didn't look at the entire file to confirm.)

Comment 10

[Gregory Szorc [:gps]]

I'm going to move this into the Testing product because that seems like a better fit. It will also get more attention from the test harness peeps.

Comment 11

[Mike de Boer [:mikedeboer]]

Gerv, Patch 1 is an implementation of the Unit Testing/1.0 spec from http://wiki.commonjs.org/wiki/Unit_Testing/1.0 (CommonJS).
The first reference implementation of this spec was implemented in Narwhal, a server-side Javascript platform, which open source, with copyright.
The source code of the original implementation can be found here: https://github.com/280north/narwhal/blob/master/packages/narwhal-lib/lib/assert.js, and mentions 'MIT' license.

Even though not much to none of the source code has remained, the original implementation still requires a copyright notice. At least, that is what the NodeJS implementation let me to believe: https://github.com/joyent/node/blob/master/lib/assert.js

The unit tests in Patch 1 are adapted from https://github.com/280north/narwhal/blob/master/tests/commonjs/assert.js, which doesn't mention a license and/ or copyright notice and later https://github.com/joyent/node/blob/6101eb184db77d0b11eb96e48744e57ecce4b73d/test/simple/test-assert.js. The exact history of the development of these unit tests are hard to assess, as well as where the original copyright lies. The version from NodeJS does require a license header, whereas the CommonJS one doesn't. I think that Joyent's copyright claim on those unit tests is false, but do they need to change that/ be aware if we want to include the tests?

Are my questions relevant to you?

Comment 12

[Mike de Boer [:mikedeboer]]

s/let/led

Comment 13

[Gervase Markham [:gerv]]

mikedeboer: When you wrote your code, did you copy any code from anywhere else? By "copy", I mean as in "copy-and-paste", or "type out again", but _not_ as in "inspired by". Comment 11 suggests that you started with code from Narwhal - is that correct? If so, I think you should put an MIT licence on it.

If the tests are based on the code you mention, https://github.com/joyent/node/blob/master/test/simple/test-assert.js, you should also put an MIT licence on them.

Gerv

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 770769 [details] [diff] [review]
Patch 1: introduce Assert.jsm, including raw xpcshell test

Tim, I hereby dub thee JavaScript Expert(tm). Would you like to review this piece of machinery?

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to Gregory Szorc [:gps] from comment #8)
> How about a compromise.
> 
> Let's export "Assert" and a utility function "export_assertions" (or
> similar). If tests so want, they can |export_assertions();|, which will
> define all the Assert.* symbols on the global this.
> 
> Thoughts?

I Like. I will implement this in the next iteration!

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 770769 [details] [diff] [review]
Patch 1: introduce Assert.jsm, including raw xpcshell test

Blair, could you review this patch? Thanks so much!

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Reinventing this wheel yet again doesn't seem useful. As usual, I'd prefer standardizing on testharness.js.

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to :Ms2ger from comment #17)
> Reinventing this wheel yet again doesn't seem useful. As usual, I'd prefer
> standardizing on testharness.js.

I *think* you're confusing this bug with bug 867742, which is where I propose a unified test harness.

This bug is not about re-inventing the wheel. It's about adopting a widely used (see URL for a list of projects) standard for Javascript assert functions in our unit tests. Introducing (newish) code from outside the m-c codebase does not necessarily imply re-inventing the wheel; in this case I truly believe that conforming to a sane standard is a matter of common sense.
I've checked out all the methods for doing assertions in our codebase (including testharness.js you mentioned). The conclusion I reached is that from the interface level they are all very similar, but from the levels of implementation and framework integration they are all very different and incoherent. I knew about the existence of a CommonJS standard for unit test assertions that solves this problem and this patch is the result.
I'm not trying to say that other test frameworks/ harnesses have been doing it wrong all this time, but instead I say that they can and should provide a better common/ unified interface for something mundane as assertions.

Additionally, as a bit more off-topic remark, I believe we've mistakenly been treating assertion methods as an integral part of any JS test framework/ harness, which has resulted in unnecessary code duplication.

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: introduce Assert.jsm, including raw xpcshell test

Hi Gregory, it's been a while since I revisited this bug... so sorry! :/

I made some small modifications to the module to make reporting hooks much easier and saner for consumers. (See the integration patches for how it works now).

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: XPCShell-test integration

voilà!

Comment 21

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: Mochitest-browser integration

et surprise!

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 812671 [details] [diff] [review]
Patch 1.2: introduce Assert.jsm, including raw xpcshell test

Review of attachment 812671 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this took me so long :\

+1 to having this, and everything said in comment 18. Will be great to standardize all the frameworks to a common API finally - it trips up me a lot, let alone newcomers.

::: testing/modules/Assert.jsm
@@ +46,5 @@
> +  }
> +  return value;
> +}
> +
> +function truncate(s, n) {

Nit: A single letter does not count as a descriptive name.

@@ +54,5 @@
> +    return s;
> +  }
> +}
> +
> +function getMessage(self) {

Nit: Either make |self| a |this|, or rename it to |error|

@@ +86,5 @@
> +  // The part of the stack that comes from this module is not interesting.
> +  let stack = Components.stack;
> +  do {
> +    stack = stack.caller;
> +  } while(stack.filename && stack.filename.indexOf("Assert.jsm") > -1)

Guess this is good enough, but use .contains() instead of indexOf()

@@ +100,5 @@
> +    configurable: true
> +  }
> +});
> +
> +(function() {

Since this is a JSM, you don't really need to hide things in a closure like this.

@@ +127,5 @@
> +   *        (string) Short explanation of the expected result
> +   * @param operator (optional)
> +   *        (string) Operation qualifier used by the assertion method (ex: '==')
> +   */
> +  this.report = function(failed, actual, expected, message, operator) {

You say things like |operator| are optional, but if you don't pass it in, it looks like the reporter will print "undefined"

@@ +204,5 @@
> +  this.deepEqual = function deepEqual(actual, expected, message) {
> +    this.report(!_deepEqual(actual, expected), actual, expected, message, "deepEqual");
> +  };
> +
> +  function _deepEqual(actual, expected) {

Why can't this just be |JSON.stringify(actual) == JSON.stringify(expected)| ? Document the differences.

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 812671 [details] [diff] [review]
Patch 1.2: introduce Assert.jsm, including raw xpcshell test

Review of attachment 812671 [details] [diff] [review]:
-----------------------------------------------------------------

Blair and I have a lot of feedback! I trust you to do "the right thing" before landing. I can't wait for this to land!

Please avoid the extra moz.build in the test directory. Instead, have the testing/modules/moz.build reference the xpcshell.ini 2 dirs deep. This will speed up the build system if it becomes a common practice. Please spread the word!

::: testing/modules/Assert.jsm
@@ +76,5 @@
> + * understood by the spec. Implementations or sub modules can pass
> + * other keys to the AssertionError's constructor - they will be
> + * ignored.
> + */
> +this.Assert.AssertionError = function AssertionError(options) {

Nit: You don't need to name functions on the right side of an equals. The engine is smart enough to inherit the name of the thing being assigned to.

@@ +137,5 @@
> +    });
> +    if (!this._reporter) {
> +      // If no custom reporter is set, throw the error.
> +      if (failed)
> +        throw err;

Nit: Always use braces.

@@ +358,5 @@
> +   *        (function) Function block to evaluate and catch eventual thrown errors
> +   * @param expected (optional)
> +   *        (mixed) Test reference to evaluate against the thrown result from `block`
> +   * @param message (optional)
> +   *        (string) Short explanation of the expected result

Can you write some more docs here? Perhaps some example usage. Things like how "expected" work aren't obvious unless the read the implementation.

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 812674 [details] [diff] [review]
Patch 2.2: XPCShell-test integration

Review of attachment 812674 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

::: testing/xpcshell/head.js
@@ +358,5 @@
> +  let assertImpl = new Assert(function(err, message, stack) {
> +    if (err)
> +      do_report_result(false, err.message, err.stack);
> +    else
> +      do_report_result(true, message, stack);

Nit: braces!

@@ +363,5 @@
> +  });
> +  // Allow Assert.jsm methods to be tacked to the current scope.
> +  this.export_assertions = function() {
> +    for (let func in assertImpl)
> +      this[func] = assertImpl[func].bind(assertImpl);

Nit: braces

Comment 25

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.3: introduce Assert.jsm, including raw xpcshell test

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.3: XPCShell-test integration

Nits addressed, carrying over r=gps.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.2: Mochitest-browser integration

Comment 28

[:Gavin Sharp [email: [e-mail]]]

(In reply to Gregory Szorc [:gps] from comment #8)
> Let's export "Assert" and a utility function "export_assertions" (or
> similar). If tests so want, they can |export_assertions();|, which will
> define all the Assert.* symbols on the global this.
> 
> Thoughts?

This means we don't actually benefit from the consolidation without test opt-in. Is that going to be required for some other reason (incompatibility)? I.e. why can't we just replace the existing ok/is/etc. functions with the ones exposed by Assert?

Comment 29

[Gregory Szorc [:gps]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #28)
> This means we don't actually benefit from the consolidation without test
> opt-in. Is that going to be required for some other reason
> (incompatibility)? I.e. why can't we just replace the existing ok/is/etc.
> functions with the ones exposed by Assert?

Followup bugs? I was originally thinking I don't want to bloat scope of this patch: it's hard enough as is.

Comment 30

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 819758 [details] [diff] [review]
Patch 1.3: introduce Assert.jsm, including raw xpcshell test

Review of attachment 819758 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/modules/Assert.jsm
@@ +56,5 @@
> +
> +function getMessage(error) {
> +  return truncate(JSON.stringify(error.actual, replacer), 128) + " " +
> +         (error.operator ? error.operator + " " : "") +
> +         truncate(JSON.stringify(error.expected, replacer), 128);

Nit: These magic numbers should be in a const somewhere.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Gregory Szorc [:gps] from comment #29)
> (In reply to :Gavin Sharp (email [e-mail]) from comment #28)
> > This means we don't actually benefit from the consolidation without test
> > opt-in. Is that going to be required for some other reason
> > (incompatibility)? I.e. why can't we just replace the existing ok/is/etc.
> > functions with the ones exposed by Assert?
> 
> Followup bugs? I was originally thinking I don't want to bloat scope of this
> patch: it's hard enough as is.

+1, in the interests of actually getting this landed.

Comment 32

[Mike de Boer [:mikedeboer]]

Pushed patches 1.3 and 2.3 to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/7d7d59c0e0fa
https://hg.mozilla.org/integration/fx-team/rev/0e9d8d7405da

Please leave this bug open, because the Mochitest part still needs to be reviewed.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d7d59c0e0fa
https://hg.mozilla.org/mozilla-central/rev/0e9d8d7405da

Comment 34

[Mike de Boer [:mikedeboer]]

Ted, review ping?

Comment 35

[(not currently active) Ted Mielczarek]

Yikes, sorry this has been sitting so long! I will review it today.

Comment 36

[(not currently active) Ted Mielczarek]

Comment on attachment 819761 [details] [diff] [review]
Patch 3.2: Mochitest-browser integration

Review of attachment 819761 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the long review delay, that's inexcusable. This looks really great, it's awesome to see a standard for this. It will be nice if we can move all our JS-based test harnesses to use this so that we don't have subtly different ways of writing tests depending on the test suite.

::: testing/mochitest/browser-test.js
@@ +451,5 @@
> +      }
> +      currentTest.addResult(res);
> +    });
> +
> +    // Allow Assert.jsm methods to be tacked to the current scope.

Longer-term would you expect this to become the default? I assume this is just to allow a transition? It would be nice if we could default new tests to using this, but I guess that might be more pain than it's worth.

::: testing/modules/moz.build
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  XPCSHELL_TESTS_MANIFESTS += ['tests/xpcshell/xpcshell.ini']
> +DIRS += ['tests/browser']

You don't need a whole new moz.build for this, you can just put a relative path in BROWSER_CHROME_MANIFESTS here, like XPCSHELL_TESTS_MANIFESTS above.

::: testing/modules/tests/browser/browser_test_assert.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> +http://creativecommons.org/publicdomain/zero/1.0/ */
> + 

nit: whitespace

::: testing/modules/tests/browser/head.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

Is this file expected to contain something in the future?

Comment 37

[Mike de Boer [:mikedeboer]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #36)
> Sorry for the long review delay, that's inexcusable. This looks really
> great, it's awesome to see a standard for this. It will be nice if we can
> move all our JS-based test harnesses to use this so that we don't have
> subtly different ways of writing tests depending on the test suite.

\o/ I was hoping you'd say that :)

I hope to deprecate `export_assertions()` soon in favor of having the Assert function accessible from the global scope directly, but we (gps, Unfocused) decided against that for now to limit the scope a bit.
I will file a follow-up to make this happen, throw some try-runs at it and see what happens!

> ::: testing/mochitest/browser-test.js
> @@ +451,5 @@
> > +      }
> > +      currentTest.addResult(res);
> > +    });
> > +
> > +    // Allow Assert.jsm methods to be tacked to the current scope.
> 
> Longer-term would you expect this to become the default? I assume this is
> just to allow a transition? It would be nice if we could default new tests
> to using this, but I guess that might be more pain than it's worth.

Yup!

Pushed to fx-team with your comments addressed: https://hg.mozilla.org/integration/fx-team/rev/e7f280616165

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7f280616165

Comment 39

[Gregory Szorc [:gps]]

Mike: Is it time to advertise this to mailing lists? It's your baby to brag about!

Comment 40

[Mike de Boer [:mikedeboer]]

Added relevant documentation to the following, relevant MDN pages:

 - https://developer.mozilla.org/en-US/docs/Mochitest
 - https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests
 - https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules
 - https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Assert.jsm

A post to dev-platform is in the making.

Comment 41

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #40)
> Added relevant documentation to the following, relevant MDN pages:
> 
>  - https://developer.mozilla.org/en-US/docs/Mochitest
>  -
> https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-
> based_unit_tests
>  - https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules
>  -
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> Assert.jsm
> 
> A post to dev-platform is in the making.

If these are the "recommended" ways of asserting things in tests, why aren't we global'ing these assert methods instead of the direct mochitest ones? All this extra typing seems silly.

Comment 42

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #41)
> If these are the "recommended" ways of asserting things in tests, why aren't
> we global'ing these assert methods instead of the direct mochitest ones? All
> this extra typing seems silly.

See comment 28 and comment 29. I'll file a follow-up to make Assert.jsm methods the default.

Comment 43

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #42)
> (In reply to :Gijs Kruitbosch from comment #41)
> > If these are the "recommended" ways of asserting things in tests, why aren't
> > we global'ing these assert methods instead of the direct mochitest ones? All
> > this extra typing seems silly.
> 
> See comment 28 and comment 29. I'll file a follow-up to make Assert.jsm
> methods the default.

Can we get that done before recommending the Assert.foo versions? Re-updating all the tests after we make these the default seems silly, too. :-)

Comment 44

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #43)
> Can we get that done before recommending the Assert.foo versions?
> Re-updating all the tests after we make these the default seems silly, too.
> :-)

I believe there's a difference in recommending something and updating all the tests.