1080457 - We need a standard way to fail tests from non-test code

Status: NEW

(Testing :: Mochitest, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

There are many behaviors that we are willing to accept when deployed but that should cause test failures. We need a standard way to do this.

At the moment, the only way to do this, afaict, is to call `Promise.reject`. We need a better solution.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

A possible API:

1/ Shipped code

Cu.import("resource://gre/modules/TestAPI.jsm", this);
TestAPI.fail("devtools", () => new Error("Oops, I shouldn't have reached this place"));

//
// This has no effect at runtime, but if this code is executed during a test, it causes
// a test failure.
//

2/ Testing code

//
// Module TestAPI is implicitly imported by xpcshell and mochitest, just like Assert.jsm
//

//
// This test triggers "devtools" error "Oops, I shouldn't have reached this place", that's by design.
//
TestAPI.expected("devtools", /Oops, I shouldn't have reached this place/);

//
// This test is also known to trigger "fx-team" error "Could not open file so and so", that's not normal
// and it should be investigated.
//
TestAPI.shouldBeFixed("fx-team", /Could not open file .*, device busy/);


----

Possible variants: 
- we could actually merge TestAPI into Assert.jsm;
- the module could also offer an API for assertion failures (based in part on nsIDebug), which would also be useful.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Any thought, Ted?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

I realize that we can certainly merge everything into Assert.jsm

// Shipped code
Assert.get("devtools").ok(false, "Oops, I shouldn't have reached this place");

// Test suite
Assert.whitelist("devtools", /Oops, I shouldn't have reached this place/);


I also realize that having this would be very nice to help move from Promise.jsm to DOM Promise, which is part of next week's program.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Possible API

First draft of an API.

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8503668 [details] [diff] [review]
Possible API

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

I must say that I love this extension to Assert.jsm; it delivers a rather elegant way to do runtime assertions in JS - a feature we're very much used to having available in C/C++.
There are plenty of areas of our JS codebase that are complex enough to warrant the need for runtime assertions and more versatile unit tests.

David, I think you came up with quite the elegant API here. I'm looking forward to seeing the implementation!

Comment 6

[Mike de Boer [:mikedeboer]]

+1 for using Assert.jsm for this work.

Comment 7

[Mike de Boer [:mikedeboer]]

Oh, as a side-note: Assert.jsm currently lives in testing/modules/, which maps to the unit-test only 'testing-common' namespace. You'd need to move Assert.jsm to toolkit to make it available for general consumption, I think.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Implementation

This implements the changes for xpcshell, chrome browser and everything based on SpecialPowers (last one is not tested yet).

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8505422 [details] [diff] [review]
Implementation

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

I like it! Thanks for working on this.

I have a few question below.

::: toolkit/components/assert/Assert.jsm
@@ +27,5 @@
>  let Assert = this.Assert = function(reporterFunc) {
> +  /**
> +   * The function used to report errors to the test suite.
> +   */
> +  this._reporter = cuReporter;

I think you'll want to set this default reporter somewhere else... for example, why not do:

```js
this.setReporter(reporterFunc);
// And change the following:
proto.setReporter = function(reporterFunc = cuReporter) {
...
```

...or something like that?

@@ +95,5 @@
>   * new assert.AssertionError({
>   *   message: message,
>   *   actual: actual,
>   *   expected: expected,
> + *   operator: operator,

I think you changed something here while WIP...

@@ -91,5 @@
> - *
> - * At present only the four keys mentioned above are used and
> - * understood by the spec. Implementations or sub modules can pass
> - * other keys to the AssertionError's constructor - they will be
> - * ignored.

This comment doesn't apply anymore?

@@ -104,5 @@
> -  let stack = Components.stack;
> -  do {
> -    stack = stack.caller;
> -  } while(stack.filename && stack.filename.contains("Assert.jsm"))
> -  this.stack = stack;

So this stack unwinding doesn't make sense here anymore... why?

@@ +614,5 @@
> + *        a known failure. Otherwise, use the default handling for
> + *        this error (e.g. a test failure).
> + */
> +proto.whitelist = function(gravity, messages) {
> +  if (!["expected", "FIXME"].indexOf(gravity) == -1) {

Shouldn't we formalize this a bit more to be constants or something?

@@ +617,5 @@
> +proto.whitelist = function(gravity, messages) {
> +  if (!["expected", "FIXME"].indexOf(gravity) == -1) {
> +    throw new TypeError("Expected either 'expected' or 'FIXME'");
> +  }
> +  if (!messages || typeof messages != "object" || messages.constructor.name != "RegExp") {

Since we have it here, you might as well make this `instanceOf(messages, "RegExp")`

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: RuntimeAssert mechanism

Here is a new version.
Patching further, I realized that I was putting into Assert.jsm many things that just didn't fit, so I decided to move the runtime behavior into RuntimeAssert.jsm.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Also, I wonder if we shouldn't rename RuntimeAssert.jsm to Warning.jsm.

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11)
> Also, I wonder if we shouldn't rename RuntimeAssert.jsm to Warning.jsm.

FWIW, I think RuntimeAssert is more aptly named than Warning.jsm

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Actually, I realize that the main usecase for this will be to replace (progressively) all calls to Cu.reportError. So maybe `ReportError.jsm`?

Comment 14

[David Teller [:Yoric] - still alive but not very active]

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> @@ -104,5 @@
> > -  let stack = Components.stack;
> > -  do {
> > -    stack = stack.caller;
> > -  } while(stack.filename && stack.filename.contains("Assert.jsm"))
> > -  this.stack = stack;
> 
> So this stack unwinding doesn't make sense here anymore... why?

I realize that I didn't answer your question. This stack unwinding broke an essential invariant of instances of `Error`, which is that property `stack` contains a string. Here, it replaced `stack` with a non-standard `nsIStackFrame`, which would eventually have bitten us.

(applied the rest of your feedback)

Comment 15

[Mike de Boer [:mikedeboer]]

David, your latest patch(es) confuse me a bit as to what you think the end-user API should be. Can you provide an example that shows its use and merit in an existing JSM?

Comment 16

[David Teller [:Yoric] - still alive but not very active]

A few examples from the top of my head:

http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Deprecated.jsm?from=Deprecated.jsm#79

Replace Cu.reportError by a call to `RuntimeAssert.fail("Deprecation", textMessage)`. This will let us progressively get rid of code that calls deprecated stuff.

Similarly, any code that currently calls `foo.then(null, Cu.reportError)`, where `foo` is a promise` should be replaced with `foo.then(null, ex => RuntimeAssert.fail("ModuleName", ex))`.

etc.

Comment 17

[Adam Roach [:abr]]

(In reply to David Rajchenbach-Teller [:Yoric] (away until November 17th - use "needinfo") from comment #16)
> Similarly, any code that currently calls `foo.then(null, Cu.reportError)`,
> where `foo` is a promise` should be replaced with `foo.then(null, ex =>
> RuntimeAssert.fail("ModuleName", ex))`.


Tiny nit; but for readability, I think you really want:

  foo.catch(ex => RuntimeAssert.fail("ModuleName", ex));

Comment 18

[Adam Roach [:abr]]

Comment on attachment 8510299 [details] [diff] [review]
RuntimeAssert mechanism

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

I wanted to read through what you're planning here, since I think your proposal is a big improvement over the current state of affairs, and am curious about the details. I don't have anything of great importance to comment on (other than general encouragement for the direction this is going), but I found some really tiny nits while I was skimming over the patch.

::: testing/modules/Assert.jsm
@@ +491,5 @@
> + *   } catch (ex) {
> + *     assert.fail(ex);
> + *   }
> + * });
> + * 

Trailing WS

@@ +509,5 @@
> + * setTimeout(() => {
> + *   try {
> + *     f();
> + *   } catch (ex) {
> + *     aseert.warn(ex);

Typo: "assert"

@@ +512,5 @@
> + *   } catch (ex) {
> + *     aseert.warn(ex);
> + *   }
> + * });
> + * 

Trailing WS

::: toolkit/components/assert/RuntimeAssert.jsm
@@ +15,5 @@
> + *   the error causes a test failure;
> + * - individual tests may whitelist individual failures, typically when
> + *   they intentionally place the code in such unexpected states, e.g.
> + *   to test recovery from errors, or deprecated codepaths.
> + * 

Trailing WS

@@ +43,5 @@
> + * Assert.whitelist("Some other subsystem").expected("Please do not call myDeprecatedFunction");
> + *
> + * // We just added a new kind of error, and we haven't fixed this test yet,
> + * // please fix this test as soon as possible.
> + * Assert.whitelist("Yet another subsystem").FIXME("FooBar has not been initialized yet");

Perhaps add an example for the ...FIXME("Error text", test_function) form? I suspect we want to encourage whitelisting errors for a short a period as possible, rather than trying to get people to apply them to entire testsuites at a time.

::: toolkit/components/assert/tests/mochi/browser_runtimeassert.js
@@ +108,5 @@
> +    Assert.equal(result.error.message, salted.message);
> +    Assert.ok(result.failed, "`fail` outside of the FIXME causes a failure.");
> +    Assert.ok(result.fatal, "`fail` outside of the FIXME is fatal.");
> +
> + 

Trailing WS

::: toolkit/components/assert/tests/xpcshell/test_runtimeassert.js
@@ +108,5 @@
> +    Assert.equal(result.error.message, salted.message);
> +    Assert.ok(result.failed, "`fail` outside of the FIXME causes a failure.");
> +    Assert.ok(result.fatal, "`fail` outside of the FIXME is fatal.");
> +
> + 

Trailing WS

Comment 19

[Jesse Ruderman]

Using the word "assert" for something that isn't necessarily a bug is really confusing, especially since we already have non-fatal and fatal assertions. This should be called "error" or "warning".

I'd like NS_WARNING and "JavaScript strict warning" to trigger this mechanism. They're the majority of console spew, and including them will benefit the entire codebase rather than just new code.

This is intended to "fail" all test suites, including their browser startup and shutdown, right? But not fuzzing?

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 8510299 [details] [diff] [review]
RuntimeAssert mechanism

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

Sorry for the ridiculous review lag here, I had a lot going on and this was just complicated enough for me to never find time to get through it. Overall this looks good, definitely better than the mess we have with trying to watch the error console etc. I have a few specific qualms with things in this patch, I don't think any of it is that big of a deal though.

::: testing/modules/Assert.jsm
@@ +101,2 @@
>    this.name = "AssertionError";
> +  if (typeof options == "string") {

I guess having this localized to Assert.jsm is not the worst thing, but it still kind of sucks.

@@ +484,5 @@
> +/**
> + * Fail, with an error message.
> + *
> + * ```js
> + * setTimeout(() => {

Not sure what the setTimeout adds to the example usage here.

::: testing/xpcshell/head.js
@@ +31,4 @@
>  // Pass a custom report function for xpcshell-test style reporting.
> +let Assert = new AssertCls(function(failed, fatal, error) {
> +  // Clients expect something that looks like a nsIStackFrame, so
> +  // let's build one.

Boy, I am not a big fan of this code. Is there really no better way to do this? Is there a platform fix that could remove the need for this code?

@@ +432,5 @@
>    // Tack Assert.jsm methods to the current scope.
>    this.Assert = Assert;
> +  for (let key in Assert) {
> +    if (!key.startsWith("_") && typeof Assert[key] == "function") {
> +      this[key] = Assert[key].bind(Assert);

Couldn't you fix this with some Object.defineProperty in Assert.jsm using enumerable: false?

::: toolkit/components/assert/RuntimeAssert.jsm
@@ +121,5 @@
> +   * This method is called by the test suite. Most users should never need to call it.
> +   *
> +   * @param {Assert} assert An instance of the testsuite constructor `Assert`.
> +   */
> +  set testsuite(assert) {

This feels oddly named. You're setting the testsuite, but you're expected to set it to an Assert instance. I can understand your logic here (you're using it as a boolean of whether you're in a testsuite run or not), but it's confusing in practice.

Comment 21

[(not currently active) Ted Mielczarek]

(In reply to Jesse Ruderman from comment #19)
> Using the word "assert" for something that isn't necessarily a bug is really
> confusing, especially since we already have non-fatal and fatal assertions.
> This should be called "error" or "warning".

I do echo Jesse's concerns with naming here--"assert" has a pretty specific meaning in our codebase, and overloading that may not be beneficial.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

I have no issue renaming "assert" to anything reasonable. Since "Error" is already taken in JS, I'd go for "Warning".

(In reply to Jesse Ruderman from comment #19)
> This is intended to "fail" all test suites, including their browser startup
> and shutdown, right?

That's the idea, yes.

> But not fuzzing?

I admit I have not given any thought to fuzzing. How is it executed?

> I'd like NS_WARNING and "JavaScript strict warning" to trigger this
> mechanism. They're the majority of console spew, and including them will
> benefit the entire codebase rather than just new code.

I'd like it, too. I suspect that fixing all warnings will require lots of work, so I'll file this as followup bugs.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: bz://1080457/Yoric

/r/9111 - Bug 1080457 - Warnings.jsm;r=ted

Pull down this commit:

hg pull -r 109d8f011d2cf46ce09579177e3b0754a5f15c01 https://reviewboard-hg.mozilla.org/gecko/

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8609374 [details]
MozReview Request: bz://1080457/Yoric

/r/9111 - Bug 1080457 - Warnings.jsm;r=ted

Pull down this commit:

hg pull -r 30e283ff3c2f79e50e5e0ada56814aab8693d823 https://reviewboard-hg.mozilla.org/gecko/

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8609374 [details]
MozReview Request: bz://1080457/Yoric

/r/9111 - Bug 1080457 - Warnings.jsm;r=ted

Pull down this commit:

hg pull -r 8870a9a7059859e81166a23e47d47cdb01b193d5 https://reviewboard-hg.mozilla.org/gecko/

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8609374 [details]
MozReview Request: bz://1080457/Yoric

/r/9111 - Bug 1080457 - Warnings.jsm;r=ted

Pull down this commit:

hg pull -r 2f7ead2d89717367e96ea1e946b0425ce18fa764 https://reviewboard-hg.mozilla.org/gecko/

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8609374 [details]
MozReview Request: bz://1080457/Yoric

/r/9111 - Bug 1080457 - Warnings.jsm;r=ted

Pull down this commit:

hg pull -r 2f7ead2d89717367e96ea1e946b0425ce18fa764 https://reviewboard-hg.mozilla.org/gecko/

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1080457 - Warnings.jsm;r=ted

Comment 30

[(not currently active) Ted Mielczarek]

Comment on attachment 8618398 [details]
MozReview Request: Bug 1080457 - Warnings.jsm;r=ted

https://reviewboard.mozilla.org/r/9111/#review11989

::: testing/mochitest/browser-test.js:701
(Diff revision 4)
> +        if (typeof func == "function") {

Couldn't you just make the non-function properties of Assert non-enumerable?

::: testing/modules/Assert.jsm:437
(Diff revision 4)
> + *     aseert.warn(ex);

typo: assert

::: testing/xpcshell/head.js:31
(Diff revision 4)
> -    do_report_result(false, err.message, err.stack);
> +  // let's build one.

This code is really ugly. Do we really need to have stack frame parsing in so many places?

::: testing/modules/Assert.jsm:426
(Diff revision 4)
> +  this._report(true, true, new Assert.AssertionError(error));

Having report take two boolean parameters in a row is pretty hard to read.

::: testing/modules/Assert.jsm:226
(Diff revision 4)
> +  this._report(failed, fatal, err);

You're changing the parameters to the reporter function, but you didn't change the documentation in the comment for setReporter.

I'm also having a hard time understanding the use of "gravity". When are we handling non-fatal assertion failures without a reporter?

::: testing/specialpowers/content/specialpowersAPI.js:621
(Diff revision 4)
> +        if (fatal) {

`fatal` seems like an odd naming choice unless you're going to terminate the browser/test run on these failures.

Comment 31

[(not currently active) Ted Mielczarek]

I'm sorry for taking so long to review this, but this is a pretty complicated patch. I'm not really comfortable with it as written, there are some parts I don't really understand the motivation for.

Comment 32

[Neha Kochar [:neha]]

Andrew, would you or someone be able to review this patch? It's very old now.

Comment 33

[Andrew Halberstadt [:ahal]]

Judging by the age of this patch and Ted's last comment, I think this will need a new owner to rebase, fix conflicts and address Ted's previous comments. I'm also not very comfortable reviewing things in this area, though at this point I'm also not sure who is :/. I think any review from someone on our team would mostly be a rubberstamp.

Comment 34

[David Teller [:Yoric] - still alive but not very active]

I've audited the code and it seems to be pretty deeply bitrotten.
I still believe that this would be a very useful feature, but getting it to work might need a full rewrite.

I think I can quickly hack together a small change from within PromiseTestUtils to achieve the same feature. The API and error messages will be a bit more surprising, but it will be simpler to land.

Comment 35

[David Teller [:Yoric] - still alive but not very active]

I won't have time to finish this.