608488 - Unit tests should run in deterministic order

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, enhancement, P1)

Description

[Drew Willcoxon :adw]

Test functions in a test file are attached to the exports object.   So not only is the order you define the test functions in not necessarily the order they're run in, but the order they're run in can potentially change between suite runs, and certainly the order for one person can potentially be different from the order for another person.

This makes it difficult to isolate failing functions from non-failing ones when failures occur because of interactions between functions.  It's potentially one reason one person sees test failures but another person doesn't.

In general unit tests should eliminate non-determinism.  Non-determinism can be useful, but if we introduce it, let's introduce it under controlled conditions.

Comment 1

[Atul Varma [:atul]]

Does running them in alphabetic order within each JS file (i.e., within each exports object) sound reasonable?  And then we could run tests for each JS file alphabetically by filename, if we don't already do that...

Comment 2

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 3

[Drew Willcoxon :adw]

I think it would be better to force the test author to determine the order, e.g., by constructing an array or appending tests to a queue.  Anything else is laying traps IMO, especially for careless authors.

Comment 4

[Matěj Cepl]

(In reply to comment #3)
> I think it would be better to force the test author to determine the order,
> e.g., by constructing an array or appending tests to a queue.  Anything else is
> laying traps IMO, especially for careless authors.

I don't think naming the modules reasonably (or using test-01-name.js convention) is more complicated than messing with yet another API for queue managing. Just my CZK0.02

Comment 5

[Drew Willcoxon :adw]

Nobody said anything about yet another API for queue managing.

Comment 6

[Matěj Cepl]

(In reply to comment #5)
> Nobody said anything about yet another API for queue managing.

By API I meant "In order to make tests run in some order you have to create this huge list / object". Did I say I hate huge lists? (see bug 635748 for another example of the similar problem, a huge switch in that case).

Comment 7

[Drew Willcoxon :adw]

(In reply to comment #6)
> Did I say I hate huge lists?

Did I say anything about huge lists?

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(automatic reprioritization of 1.0 bugs)

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Re-prioritizing all 1.1-targeted feature requests to 1.2.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Another option would be to use a custom module loader to provide test modules with a proxied `exports` object that captures property sets and appends test function names to the runner's queue.

The runner would then call the functions in the order in which they are evaluated, which seems to be what test developers expect, without requiring those developers to explicitly define the queue themselves.

Using a custom loader might complicate our plans to simplify the module loader, although we could build one for just this particular case instead of using the existing generic support for custom loaders, which we've been talking about removing (or perhaps have already removed; I can't remember now).

Brian: any thoughts on using a custom loader for this case?

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Actually cc:ing Brian this time.

Brian: see my question for you in comment 10!

Comment 12

[Brian Warner [:warner :bwarner]]

Hm.. feels a bit complicated. I guess I'm -0 on going that way.

I suppose the new 'exports' object itself wouldn't be too bad.. it'd have to
be a Harmony "Proxy" object, right?, so you could intercept arbitrary
property sets, and maybe append their names to an "_ordered_propnames"
property.

We'd want to think about the security concerns of that, but off the top of my
head they don't sound too bad. We should make it immutable, just like the
'exports' object itself, to prevent two importers of a shared module from
using that module for communication (assuming the module doesn't provide
communication directly). But we aren't exposing any new secrets to the
require() side, and I can't think offhand of any way to trick either side by
putting weird things in _ordered_propnames.

I *am* a bit concerned about object-identity comparisons that now get handed
the Proxy instead of the underlying exports container. And will there be
slowdowns because every require()d object now has to pass through a Proxy?

I'm not sure how to limit its effect to test cases.. add a flag to the
loader, I suppose. The unit-test driver needs a special interface to
(currently) search for modules on disk, or (eventually) enumerate the
manifest, which it can use to then require() the test cases. So we'd either
need to change the signature of the standard require(), which might run into
CommonJS compatibility problems later if they do the same thing, or add a new
magic interface function for this like require2(name, flags). I'd be inclined
to just let it affect all modules, rather than adding a special case.

If we use a new magic interface, we could allow require() to return the
normal exports object

I see three approaches here:

 1: run tests in alphabetical order
 2: run tests in order of defineProperty()
 3: obey e.g. exports.orderedTestNames=["test_foo", "test_bar"]

Personally, I'd lean towards just running tests in alphabetical order: it's
easy to explain, involves no magic, and makes it possible (albeit inelegant)
for developers to control the order of tests if and when they care about it.
Also, the test frameworks I use in other languages (python's "unittest"
module) does it that way. #2 is less work for developers but then the docs
have to admit that 'exports' is something magic (and some portion of
developers will get distracted with "huh, how did they do that?"). #3 is hard
to discover: you'd only ever notice it by reading the docs, whereas #1 jumps
out at you if you have more than a handful of tests.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: makes tests run alphabetically

(In reply to Brian Warner [:warner :bwarner] from comment #12)
> Personally, I'd lean towards just running tests in alphabetical order: it's
> easy to explain, involves no magic, and makes it possible (albeit inelegant)
> for developers to control the order of tests if and when they care about it.

I reluctantly agree, as the approach combines some obviousness with the advantages you list.

So here's a patch that implements this behavior.  In my testing, core tests continue to pass with this patch applied.

Comment 14

[Brian Warner [:warner :bwarner]]

Comment on attachment 558230 [details] [diff] [review]
patch v1: makes tests run alphabetically

looks good!

Comment 15

[Myk Melez [:myk] [@mykmelez]]

https://github.com/mozilla/addon-sdk/commit/20604fed251d1761a634325cd2f92e04d47281bf