867742 - Adopt TestSuite.jsm for testing

Status: RESOLVED WONTFIX

(Testing :: General, defect)

Description

[Mike de Boer [:mikedeboer]]

For a full explanation, background & details: please check out https://gist.github.com/mikedeboer/5495405 and the mailing list discussions on dev-platform and dev-firefox.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Add Async.jsm and AsyncTest.jsm modules

This patch adds the two files in one go. I *could* separate them into two patches.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Add Async.jsm as a module

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Add AsyncTest.jsm as a module

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 746383 [details] [diff] [review]
Add AsyncTest.jsm as a module

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

General comments:

* It might be worth splitting this bug into 2. I think there will be tons of bikeshedding.
* /testing/modules /might/ be a better location for this code.
* We should consider splitting the module into pieces. I like a division between the core and the reporters for a start. If nothing else, it forces you to think more about APIs.
* Need docs everywhere. A standalone README might be in order.
* What about assertion functions? I'd like to see your plan before this is committed, as they are a large part of testing and I fear that without them, this framework will become just as fragmented as our existing test suites.

I'm granting f+ because I love the idea of a generic JS testing library that's sensibly designed, which this is. I *really* want to see this land. I think you should get a lot more eyes on the next revision. Let's loop in Automation and sheriffs sooner than later: there are lots of people thinking about how to better capture test results and this library could go a long way to solving the JS pieces.

::: toolkit/modules/AsyncTest.jsm
@@ +1,2 @@
> +/* 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,

Nit: "file," should be on line 3. See https://www.mozilla.org/MPL/headers/

@@ +13,5 @@
> + * You can find more documentation of this library at:
> + * https://github.com/mikedeboer/mozAsync#asynctestjsm
> + */
> +
> +var Async, exports;

First, |"use strict";| should be in all new JSMs.

Second, I'm of the belief we should banish var from all new JSMs and require let. IMO the only time var is acceptable for new JS is if it needs to run outside of Firefox. Even content privileged code in Firefox can be loaded in a context that supports modern JS.

I see you support loading from not-XPCOM. What are your plans? Please justify why we should bother supporting running outside of Gecko if needed, as I feel that test code will likely end up being tightly coupled with our needs and the burden to maintain non-Gecko compatibility could grow unwieldy.

@@ +34,5 @@
> +    return fn;
> +  } else {
> +    return function() {
> +      var value;
> +      var next = arguments[args];

If we targeted modern JS, we could use rest parameters: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/rest_parameters

@@ +54,5 @@
> +  if (isGecko) {
> +    return dump(aMsg);
> +  }
> +  process.stdout.write(aMsg);
> +}

I think this will need to be abstracted some day. Depending on the test environment, we'll want to send logs to different places. It's fine to have a default implementation. But please allow external consumers to provide their own log function just like they can provide their own reporter. Perhaps it's best to roll logging into the reporter?

@@ +79,5 @@
> +Suite.STATE_START   = 0x0001;
> +Suite.STATE_SKIPPED = 0x0002;
> +Suite.STATE_DONE    = 0x0004;
> +Suite.STATE_END     = 0x0008;
> +Suite.STATE_TEST    = 0x0010;

Docs!

@@ +88,5 @@
> +    self.report(Suite.STATE_START);
> +
> +    Async.eachSeries(this.tests, function(test, next) {
> +      var context = test.context || self;
> +      if (!context.notify)

Nit: Always insert curly braces.

@@ +118,5 @@
> +
> +        // timeout to watch async processes running too long...
> +        var timeout = Async.setImmediate(function() {
> +          called = true;
> +          callback("Source did not respond after " + test.timeout + "ms!");

We should have a way for machines to know the test timed out without requiring parsing for this message.

@@ +142,5 @@
> +          }
> +        });
> +      }, function(err) {
> +        test.err = err;
> +        test.passed = !err;

Instead of mutating the original test object, I'd consider creating a new object to encapsulate the result.

A scenario I'd like you to consider is repeated tests and intermittent oranges. Today, AFAIK, all our test runners only run an individual test once. I would like us to get to a point where the test runners are flexible enough to assist with the intermittent detection. e.g. if a test fails, the test runner could feed the failing test back onto the tests queue. If it fails every time, it's like a permanent failure. If it cycles, then we're looking at an intermittent. Anyway, this necessitates handling multiple results per test input. So, that means you either need to store multiple state on each test. Or, you can defer that problem to "downstream" and just emit result objects.

Does that make sense?

@@ +151,5 @@
> +      self.report(Suite.STATE_END);
> +    });
> +  },
> +
> +  report: function(aState, aTest) {

I love what you've done here with abstracted reporting. There is tons of awesomeness that can be achieved by doing things this way. It opens up many doors for better tools and reporting of test results.

However, I think the API could use some work.

I think a listener pattern might be better than a single "report" API. This way, you could register multiple listeners and you establish a more formal API for each event type. There is all kinds of awesome that could come from this. As mentioned above, the "onTestResult" listener event could attach a function that requests a test be re-executed. Or, the listener could request immediate termination of the test suite. You could install 1 listener that outputs for humans and another that writes machine-readable files.

I think a more formal events API would also enforce the compartmentalization of concerns between running tests and reporting on their results. As it stands, the test runner is concerned with stats about test execution. IMO this should be in the realm of a event reactor, not part of the core test invocation logic.

@@ +175,5 @@
> +    if (aState & Suite.STATE_END)
> +      this.options.onEnd();
> +  },
> +
> +  notify: function() {

How is this different from report()?

@@ +210,5 @@
> +  var tearDown = aSuite.tearDown || null;
> +
> +  var single;
> +  methods.forEach(function(name) {
> +    if (name.charAt(0) == ">")

Huh?

{>foo: true} nets a syntax error for me.

So, you'll have to define your test functions like:

{
  ">foo": function () {...}
}

That seems annoying, especially since objects will have mixed quoting and non-quoting. Few characters, sure.

What about a naming convention that doesn't require quoting. t_*? test_*?

@@ +231,5 @@
> +      name: name.replace(/(^[>!\s]*|[\s]*$)/, ""),
> +      setUp: setUp,
> +      tearDown: tearDown,
> +      context: aSuite.tests,
> +      timeout: aSuite.timeout || aSuite.tests.timeout || 3000,

Magic numbers should be constants.

@@ +251,5 @@
> +    return suite;
> +  return suite.run();
> +};
> +
> +exports.AsyncTest.DEFAULT_REPORTER = "dot";

This is the part of the file where I mostly stopped reviewing because I think we should focus on the execution and definition library for now.

@@ +547,5 @@
> + * http://www.movable-type.co.uk/scripts/aes.html
> + *
> + * @param {String} strUni
> + */
> +function utf8encode(strUni) {

Writing this should have set off alarm bells in your head: YOU ARE REINVENTING THE WHEEL. YOU ARE REINVENTING THE WHEEL.

A proper way to perform UTF-8 encoding in Gecko is https://mxr.mozilla.org/mozilla-central/source/services/common/utils.js#177.

I'm not a Unicode expert, but I'm willing to bet money that this implementation is not proper. Unicode is more complicated than what 99% of developers realize. I've learned the hard way to not trust anything except a dedicated and well-used library.

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Comment on attachment 746383 [details] [diff] [review]
> Add AsyncTest.jsm as a module
> 
> Review of attachment 746383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General comments:
> 
> * It might be worth splitting this bug into 2. I think there will be tons of
> bikeshedding.
> * /testing/modules /might/ be a better location for this code.
> * We should consider splitting the module into pieces. I like a division
> between the core and the reporters for a start. If nothing else, it forces
> you to think more about APIs.
> * Need docs everywhere. A standalone README might be in order.
> * What about assertion functions? I'd like to see your plan before this is
> committed, as they are a large part of testing and I fear that without them,
> this framework will become just as fragmented as our existing test suites.

First of all, thanks for this massive review and the amount of time you put in! I think all your points are valid and I will do my best to address them asap.

* I will split the bug.
* The Async.jsm API is documented in a separate README at https://github.com/mikedeboer/mozAsync/blob/master/README.md. Should I move those docs to inline doc block-comments? The README should move together with the module, so put it next to it?
* Assertion functions will be added. I'm still thinking about how which Mochitest/ XPCShell assert 'special' features need to be supported/ ported, but we can discuss that on IRC or perhaps an etherpad?

> 
> I'm granting f+ because I love the idea of a generic JS testing library
> that's sensibly designed, which this is. I *really* want to see this land. I
> think you should get a lot more eyes on the next revision. Let's loop in
> Automation and sheriffs sooner than later: there are lots of people thinking
> about how to better capture test results and this library could go a long
> way to solving the JS pieces.

Yay! idea++ on looping in Automation and sheriffs. Do that on the next revision of this bug?

> Second, I'm of the belief we should banish var from all new JSMs and require
> let. IMO the only time var is acceptable for new JS is if it needs to run
> outside of Firefox. Even content privileged code in Firefox can be loaded in
> a context that supports modern JS.
> 
> I see you support loading from not-XPCOM. What are your plans? Please
> justify why we should bother supporting running outside of Gecko if needed,
> as I feel that test code will likely end up being tightly coupled with our
> needs and the burden to maintain non-Gecko compatibility could grow unwieldy.
> 

I did this to support running the test-suite from NodeJS (hence the 'var' instead of 'let').
I will move this, including its unit tests, to moz-style JS entirely in the follow-up patch.

>
> We should have a way for machines to know the test timed out without
> requiring parsing for this message.
> 

I think the timeout-reporting should move to the reporter plugins.

> 
> A scenario I'd like you to consider is repeated tests and intermittent
> oranges. Today, AFAIK, all our test runners only run an individual test
> once. I would like us to get to a point where the test runners are flexible
> enough to assist with the intermittent detection. e.g. if a test fails, the
> test runner could feed the failing test back onto the tests queue. If it
> fails every time, it's like a permanent failure. If it cycles, then we're
> looking at an intermittent. Anyway, this necessitates handling multiple
> results per test input. So, that means you either need to store multiple
> state on each test. Or, you can defer that problem to "downstream" and just
> emit result objects.
> 
> Does that make sense?
> 

That does make sense! How this can be supported as a module is something we can discuss on IRC or perhaps an etherpad?

> 
> I love what you've done here with abstracted reporting. There is tons of
> awesomeness that can be achieved by doing things this way. It opens up many
> doors for better tools and reporting of test results.
> 
> However, I think the API could use some work.
> 
> I think a listener pattern might be better than a single "report" API. This
> way, you could register multiple listeners and you establish a more formal
> API for each event type. There is all kinds of awesome that could come from
> this. As mentioned above, the "onTestResult" listener event could attach a
> function that requests a test be re-executed. Or, the listener could request
> immediate termination of the test suite. You could install 1 listener that
> outputs for humans and another that writes machine-readable files.
> 
> I think a more formal events API would also enforce the compartmentalization
> of concerns between running tests and reporting on their results. As it
> stands, the test runner is concerned with stats about test execution. IMO
> this should be in the realm of a event reactor, not part of the core test
> invocation logic.
> 

Bright idea. Will use an EventEmitter-style implementation in the next iteration. What do you prefer... nsIObservable or more in the style add-ons SDK and devtools uses?

> 
> Huh?
> 
> {>foo: true} nets a syntax error for me.
> 
> So, you'll have to define your test functions like:
> 
> {
>   ">foo": function () {...}
> }
> 
> That seems annoying, especially since objects will have mixed quoting and
> non-quoting. Few characters, sure.
> 
> What about a naming convention that doesn't require quoting. t_*? test_*?
> 

I agree.

> 
> Writing this should have set off alarm bells in your head: YOU ARE
> REINVENTING THE WHEEL. YOU ARE REINVENTING THE WHEEL.
> 
> A proper way to perform UTF-8 encoding in Gecko is
> https://mxr.mozilla.org/mozilla-central/source/services/common/utils.js#177.
> 
> I'm not a Unicode expert, but I'm willing to bet money that this
> implementation is not proper. Unicode is more complicated than what 99% of
> developers realize. I've learned the hard way to not trust anything except a
> dedicated and well-used library.

You're very right :) TBH, I copied this while hacking in the Growl notifications... will change!

Comment 6

[Gregory Szorc [:gps]]

Sorry for the delay on the reply.

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Yay! idea++ on looping in Automation and sheriffs. Do that on the next
> revision of this bug?

The sooner the better. They typically have good ideas and insights and their involvement may significantly shape the direction of this bug.
 
> Bright idea. Will use an EventEmitter-style implementation in the next
> iteration. What do you prefer... nsIObservable or more in the style add-ons
> SDK and devtools uses?

I like add-on style listeners as opposed to nsIObservers. The main reason I don't like nsIObserverService is the channel is a singleton and thus precludes certain use cases (like having multiple instances). A downside with add-on style listeners is they keep a reference around and prevent GC unless you manually keep weak references. But, I don't think that will be an issue here.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: WIP: Add TestSuite.jsm as a module

Hi Greg, I reworked quite a number of things, but not all your comments are addressed yet.

There are a couple of things I'd really like you to take a look at:
* Reworked test suites to be event emitters and reporters to listen to those events. I like it much better and hope you do too. They're quite self-documenting. The devtools event-emitter module can not be used, so included a verbatim copy into the module :(
* The question of documentation is still open, I think it's best to move it from Github/ markdown to doc-block comments inside the module
* How do I include Automation and sheriffs into this discussion so they can express their wishes/ feature requests/ must-haves?
* Please see the unit test for an example of the TestSuite syntax/ API. How does it look?
* Intermittent oranges: I think that can be abstracted away by a simple API on a suite or a 'magic string' inside the test name - like "it should never ever fail (orange,3,2s)", which will retry the test up to 3 times, with an interval of 2 seconds between each retry. The Suite implementation can deal with this specific case.
* Timeout reporting still has to move to the reporter plugins

I hope you like the progress so far!

Comment 8

[Gregory Szorc [:gps]]

Adding some sheriffs and ateam people who might be interested in this effort.

Perhaps we should move this to the Testing product?

Comment 9

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

My usual comment is that I'd be more interested in reusing testharness.js (dom/imptests in m-c, https://github.com/w3c/testharness.js/ upstream), as that's the one that I have to use for most of my tests anyway, and I like to think it has a pretty good API.

Comment 10

[cmtalbert]

I am confused. I thought the consensus on the thread where this was proposed was to NOT use this style of asynchronous javascript over the existing use of task.jsm and DOM futures/promises. [1]

So, while I didn't review everything on this bug in great depth, I do want to start out by asking why are we continuing in this vein?

That said, I'm all for aligning our JS test frameworks to use the same style, so I'd love to see that part of this carried forward, but I would rather see us promoting the use of our existing in tree async patterns since adding another just makes things complicated, which is expressly not the intention of this bug.

[1] https://groups.google.com/forum/?fromgroups#!searchin/mozilla.dev.platform/asynctest/mozilla.dev.platform/14ER9uMe8Dw/6fukwFeXNAkJ

Comment 11

[Gregory Szorc [:gps]]

I think the test harness and declaration format should largely be agnostic to whether tests are using callbacks, promises, tasks, etc. If we design things right, we will keep the door open for future methods of defining test "flavors." When tomorrow's new hotness comes around, we can add some syntactic sugar to the declaration format to easily support and be done with it.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 761525 [details] [diff] [review]
WIP: Add TestSuite.jsm as a module

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

I kinda dozed off at the end of TestSuite.jsm. I'd like to focus on the actual test running bits more than the reporter implementation for now.

I'd *really* like to see more docs on how all this works, as it is difficult to follow what's all supposed to happen. I'm specifically looking for things like:

* How will Assert.jsm get plugged in
* Native support for tasks/generator functions?
* How can you extend the TestSuite "class" so mutliple test files can share common helper functions?
* What is "this" inside a test function?

Things are looking good though. I hope we can start a review of the test running (but not reporting) bits soon.

::: testing/modules/TestSuite.jsm
@@ +26,5 @@
> +this.EXPORTED_SYMBOLS = [
> +  "TestSuite"
> +];
> +
> +function makeAsync(aArgCount, aFn, aContext) {

Nit: Don't a-prefix arguments: this is JavaScript, not C++.

@@ +29,5 @@
> +
> +function makeAsync(aArgCount, aFn, aContext) {
> +  if (aFn.length > aArgCount) {
> +    return aFn;
> +  } else {

Nit: If you have a return, you don't need the else. Some will argue the else makes branching clearer. But, I think that benefit is typically outweighed by reduced indentation from the lack of the else block.

if (x) {
  return;
}

// The rest of your function without the else.

@@ +68,5 @@
> +      .split(new RegExp("[\\s ]*" + (aSeparator || ",") + "[\\s ]*", "g"), aLimit);
> +};
> +
> +// XXXmikedeboer: This will become obsolete when we can use the devtools EventEmitter
> +function EventEmitter() {};

Is there a bug on file for making EventEmitter work with xpcshell? Perhaps you should just drop this code into EventEmitter.jsm and file a follow-up for the devtools version to consolidate with it?

@@ +79,5 @@
> +   *        The event name to which we're connecting.
> +   * @param function aListener
> +   *        Called when the event is fired.
> +   */
> +  on: function EventEmitter_on(aEvent, aListener) {

Nit: You don't need to name functions that are values in objects. All the tools will automatically use the corresponding key as the function name if no function name is defined.

@@ +100,5 @@
> +  once: function EventEmitter_once(aEvent, aListener) {
> +    let handler = function() {
> +      this.off(aEvent, handler);
> +      aListener.apply(null, arguments);
> +    }.bind(this);

You /could/ write this as:

let handler = () => {
  this.off(event, handler);
  listener.apply(null, arguments);
};

Not sure how people feel about adopting fat arrows yet...

@@ +158,5 @@
> +};
> +
> +function Suite(aTests) {
> +  this.tests = aTests;
> +  let firstTest = aTests[0];

Perhaps we should throw if !tests.length

@@ +175,5 @@
> +}
> +
> +(function() {
> +  this.run = function() {
> +    let self = this;

I'm not really a fan of aliasing this when a .bind(this) or a fat arrow function will do the same thing.

@@ +178,5 @@
> +  this.run = function() {
> +    let self = this;
> +    this.emit("suite-start");
> +
> +    Async.eachSeries(this.tests, function(test, next) {

Nit: Name this function!

@@ +201,5 @@
> +
> +      let chain = test.skip ? [test.setUpSuite, test.tearDownSuite] :
> +        [test.setUpSuite, setUp, testFn, tearDown, test.tearDownSuite];
> +
> +      self.emit("test-start");

I'm confused about the suite setup and teardown functions and test iteration.

You emit suite-start above yet register setUpSuite and tearDownSuite inside this iterator over this.tests. I would think that setUpSuite and tearDownSuite would be called outside of this iteration, independent of the calling chain.

@@ +210,5 @@
> +        // timeout to watch async processes running too long...
> +        let timeout = Async.setImmediate(function() {
> +          called = true;
> +          callback("Source did not respond after " + test.timeout + "ms!");
> +        }, test.timeout);

I'm not sure if it's necessary to have a separate timeout for each function in the chain. Can we just go with a single timeout for the entire chain?

@@ +241,5 @@
> +        next();
> +      });
> +    }, function() {
> +      self.emit("suite-end");
> +    });

Hold indentation, batman! I /think/ you are emitting suite-end after each this.tests iteration, not just once at the end of run(). If so, the suite-start and suite-end "tags" don't match up!

@@ +247,5 @@
> +
> +  this.addListeners = function() {
> +    this._listeners = {
> +      "suite-start": function() {
> +        this.stats.start = new Date();

I /think/ there is a monotonic time function available somewhere in the tree. That might be more appropriate. But, we could relegate that to a follow-up.

@@ +332,5 @@
> +  let tearDown = aSuite.tearDown || null;
> +
> +  let single;
> +  methods.forEach(function(name) {
> +    if (name.charAt(0) == ">") {

I could use a README or some comments to tell me what's going on here!

@@ +342,5 @@
> +  }
> +
> +  let reporters = [];
> +  // We support two possible options to define custom reporters:
> +  [aSuite.reporters, aSuite.reporter].forEach(function(aOption) {

Can we only support 1, please (likely an array since that doesn't constrain behavior as much).

@@ +371,5 @@
> +      context: aSuite.tests,
> +      timeout: aSuite.timeout || aSuite.tests.timeout || TestSuite.DEFAULT_TIMEOUT,
> +      fn: aSuite.tests[name],
> +      count: count,
> +      setUpSuite: i - 1 === 0 && aSuite.setUpSuite ?

i == 1

@@ +387,5 @@
> +  }
> +  return suite.run();
> +};
> +
> +this.TestSuite.DEFAULT_TIMEOUT = 3000;

Please embed units into constants. e.g. DEFAULT_TIMEOUT_MS.

@@ +390,5 @@
> +
> +this.TestSuite.DEFAULT_TIMEOUT = 3000;
> +this.TestSuite.DEFAULT_REPORTER = "dot";
> +
> +const Colors = {

I'd prefer if we separated out all this reporting foo into a separate JSM. That will reinforce the separation between running tests and reporting on the results. It might be worth creating /toolkit/testharness or something instead of stuffing everything into /toolkit/modules. Ask Mossop what he prefers.

@@ +427,5 @@
> + * that are common among reporters.
> + */
> +let Cursor = {
> +  hide: function(){
> +    log("\u001b[?25l");

Do I need to explain how terminals work and how each one has different escape sequences?

@@ +466,5 @@
> +
> +let s = 1000;
> +let m = s * 60;
> +let h = m * 60;
> +let d = h * 24;

I'd constify these instead of having cryptic single letter file-scoped variables.

@@ +514,5 @@
> + * @param {Number} ms
> + * @return {String}
> + * @api public
> + */
> +function ms(ms) {

Surely this and parse() are already implemented somewhere in the tree. I wouldn't be surprised if we even had functions somewhere that properly handled l18n issues!

::: 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 rot

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

Delete empty Makefile.in.

::: testing/modules/tests/xpcshell/test_testSuite.js
@@ +99,5 @@
> +        next();
> +      }
> +    },
> +    onEnd: onEnd
> +  }

Will definitely need tests for timeouts!

::: testing/xpcshell/xpcshell.ini
@@ +26,5 @@
>  [include:dom/indexedDB/test/unit/xpcshell.ini]
>  [include:docshell/test/unit/xpcshell.ini]
>  [include:docshell/test/unit_ipc/xpcshell.ini]
>  [include:embedding/tests/unit/xpcshell.ini]
> +[include:testing/modules/tests/xpcshell/xpcshell.ini]

The master xpcshell.ini manifest was just removed in bug 869635 \o/

Comment 13

[Mike de Boer [:mikedeboer]]

Thanks for the awesome review, I'll post a follow-up as soon as I can!

Comment 14

[Dave Townsend [:mossop]]

FWIW this feels like overkill to me. The main goal seems to be to create a standard way to write unit tests that support async in a way that isn't overly complicated and works in all test suites. Noble but I think it would be better to just extend the mechanism we already have for this (add_test/run_next_test from xpcshell) into mochitests and call it a day. Yours has some more features, but I don't think they are commonly used features and they can also be easily implemented on top of what we already have.

Comment 15

[Gregory Szorc [:gps]]

(In reply to Dave Townsend (:Mossop) from comment #14)
> FWIW this feels like overkill to me. The main goal seems to be to create a
> standard way to write unit tests that support async in a way that isn't
> overly complicated and works in all test suites. Noble but I think it would
> be better to just extend the mechanism we already have for this
> (add_test/run_next_test from xpcshell) into mochitests and call it a day.
> Yours has some more features, but I don't think they are commonly used
> features and they can also be easily implemented on top of what we already
> have.

I sympathize with where you are coming from. But I think code near to its current implementation is justified.

It's hard for me to articulate why. Many of the reasons floating around in my head seem to boil down to the fact that TestSuite.jsm allows tests to be more strictly defined while increasing flexibility for test runners to consume tests in new and novel ways. For example, test functions in TestSuite.jsm are bound to an object, not the global scope. This opens all kinds of usefulness for defining helper functions. Before, you'd have to define helper functions on the global scope. But don't leak those symbols or attempt to run another test in the same scope, because those symbols might conflict or cause a leak error (in the case of mochitests)! With TestSuite.jsm, you attach symbols to a new object and you don't have to worry about these things. Furthermore, you could attach these functions to a new prototype so tests in separate files could easily share accessory functions. That should eliminate this [head] and [tail] files BS from xpcshell tests.

If you're still not convinced, I can rattle off a few more reasons...

Comment 16

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #15)
> (In reply to Dave Townsend (:Mossop) from comment #14)
> > FWIW this feels like overkill to me. The main goal seems to be to create a
> > standard way to write unit tests that support async in a way that isn't
> > overly complicated and works in all test suites. Noble but I think it would
> > be better to just extend the mechanism we already have for this
> > (add_test/run_next_test from xpcshell) into mochitests and call it a day.
> > Yours has some more features, but I don't think they are commonly used
> > features and they can also be easily implemented on top of what we already
> > have.
> 
> I sympathize with where you are coming from. But I think code near to its
> current implementation is justified.
> 
> It's hard for me to articulate why. Many of the reasons floating around in
> my head seem to boil down to the fact that TestSuite.jsm allows tests to be
> more strictly defined while increasing flexibility for test runners to
> consume tests in new and novel ways. For example, test functions in
> TestSuite.jsm are bound to an object, not the global scope. This opens all
> kinds of usefulness for defining helper functions. Before, you'd have to
> define helper functions on the global scope. But don't leak those symbols or
> attempt to run another test in the same scope, because those symbols might
> conflict or cause a leak error (in the case of mochitests)! With
> TestSuite.jsm, you attach symbols to a new object and you don't have to
> worry about these things. Furthermore, you could attach these functions to a
> new prototype so tests in separate files could easily share accessory
> functions. That should eliminate this [head] and [tail] files BS from
> xpcshell tests.
> 
> If you're still not convinced, I can rattle off a few more reasons...

I think you're still going to need head/tail files whenever you find you need to share code across test files and since we can load test files in different sandboxes I think we can avoid the sorts of scope pollution you seem concerned about. I also don't think it's historically been a problem for us, at least not enough that it is worth spending time to solve.

I'm still not convinced, but as long as you're not looking to land this in shipping code I don't need to be. I'm just throwing my suggestion in that we could make a larger impact to unifying our test suites by just tackling the differences directly rather than coming up with something entirely new.

Comment 17

[Gregory Szorc [:gps]]

Mike: Any chance we can get this revived? Perhaps there's someone else willing to pick this up?

Comment 18

[Mike de Boer [:mikedeboer]]

(In reply to Gregory Szorc [:gps] from comment #17)
> Mike: Any chance we can get this revived? Perhaps there's someone else
> willing to pick this up?

Aye! Your previous bump in another bug yesterday brought this back on my radar... it's been kinda hectic lately.
I'm throwing this in my 'Let's do something fun now'-bucket.

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: Add TestSuite.jsm as a module

Here's a set of three patches that implement what I think needs to be done.

Please, feel free to provide feedback on them. I'm not setting flags yet.

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: Add TestReporters.jsm as a module, which are helpers for TestSuite.jsm

Comment 21

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: Add unit tests for the new TestSuite.jsm module

Comment 22

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: Add unit tests for the new TestSuite.jsm module

Added example of custom reporter.

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: Add TestSuite.jsm as a module

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: Add TestReporters.jsm as a module, which are helpers for TestSuite.jsm

Comment 25

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: Add unit tests for the new TestSuite.jsm module

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: Add TestReporters.jsm as a module, which are helpers for TestSuite.jsm