1048427 - unit-test "done" should be untangled from "ui is clean"

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

In Firefox 32 Addon-sdk, unit-test `done()` acquires the constraint that there must only be one window and one tab.

I claim this confuses the 'async' attribute of "done()", with some logical notion of 'cleaned up.  This has problems.

1.  Getting back down to "no tabs and windows" is also hard to guarantee, and error prone. 
2.  Not all test actually require UI teardown.  I often care about the state of the ui (for example, after some sequence, a tab is open and ready), but not about destroying it.  [See Note 1]

Suggestion:

a.  Helper function "promise backToOneWindowAndTab()" that removes all tabs and windows.
b.  change tests that require cleanup back to:  "cleanup.then(done)".  


.02 from someone who writes lots of addon-tests, and got burned by this.


Note 1:  An actual example.  https://github.com/raymak/contextualfeaturerecommender/commit/e72f8aaf067e75629e34577fe202303aaa5aeea3

Comment 1

[Dave Townsend [:mossop]]

(In reply to Gregg Lind (User Research - Test Pilot) from comment #0)
> In Firefox 32 Addon-sdk, unit-test `done()` acquires the constraint that
> there must only be one window and one tab.
> 
> I claim this confuses the 'async' attribute of "done()", with some logical
> notion of 'cleaned up.  This has problems.

Tests are meant to clean themselves up. Sometimes you can get away without it, but as your test suite grows you'll hit the point where not cleaning up will bite you.

> 1.  Getting back down to "no tabs and windows" is also hard to guarantee,
> and error prone.

Why is this?

> 2.  Not all test actually require UI teardown.  I often care about the state
> of the ui (for example, after some sequence, a tab is open and ready), but
> not about destroying it.  [See Note 1]
> 
> Suggestion:
> 
> a.  Helper function "promise backToOneWindowAndTab()" that removes all tabs
> and windows.

We could include this in the test suite somewhere for people to use.

> b.  change tests that require cleanup back to:  "cleanup.then(done)".

The problem is that we often can't recognise tests that require cleanup until later tests fail, then it is a difficult process to figure out which test is causing the problem. Verifying after each test simplifies this.

Comment 2

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

"Tests are meant to clean themselves up. Sometimes you can get away without it, but as your test suite grows you'll hit the point where not cleaning up will bite you."

Well, sometimes that is true.  UI cleanup isn't the only kind of cleanup.  When a text expects things to be clean before starting, that sounds like setUp problem as much as a tear down problem.  Inside the sdk itself, tests can make any assumptions that make sense, like having functions like "ensureUIisCleanBeforeStarting" or whatnot.  Those same assumptions don't make sense other places (which I can say, because for Experiment addons, like all the stuff I do for Test Pilot, and Telemetry Experiment work, it's mostly irrelevant).

Re:  Helper.
1.  It's hard to guarantee because getting back to 0 is an async process.  Another place where errors creep in.  

Also, if this hasn't been fixed in the docs, it should mentioned.  The old docs were clear about "use the two arg test version if you have async."  That needs to be amended to point out that the two arg version now requires this particular UI cleanup when it's used.

Comment 3

[Dave Townsend [:mossop]]

(In reply to Gregg Lind (User Research - Test Pilot) from comment #2)
> "Tests are meant to clean themselves up. Sometimes you can get away without
> it, but as your test suite grows you'll hit the point where not cleaning up
> will bite you."
> 
> Well, sometimes that is true.  UI cleanup isn't the only kind of cleanup. 
> When a text expects things to be clean before starting, that sounds like
> setUp problem as much as a tear down problem.  Inside the sdk itself, tests
> can make any assumptions that make sense, like having functions like
> "ensureUIisCleanBeforeStarting" or whatnot.  Those same assumptions don't
> make sense other places (which I can say, because for Experiment addons,
> like all the stuff I do for Test Pilot, and Telemetry Experiment work, it's
> mostly irrelevant).

Either a test can attempt to get itself into a sane state at the start (having to do everything from closing windows, cleaning up the toolbars, loading the default page), or any test that changes browser state can change it back again. It makes more sense to do the latter, it is less work and keeps the test well contained.

> Re:  Helper.
> 1.  It's hard to guarantee because getting back to 0 is an async process. 
> Another place where errors creep in.  

I don't understand what about async makes this hard to guarantee

> Also, if this hasn't been fixed in the docs, it should mentioned.  The old
> docs were clear about "use the two arg test version if you have async." 
> That needs to be amended to point out that the two arg version now requires
> this particular UI cleanup when it's used.

Both should need it. But you're right the docs should be fixed.

Comment 4

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

P2, patches welcome to either not throw the error in normal tests outside of the SDK tests, or to add some flag to not throw the error for particular tests.

Comment 5

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

In both our proposals, a helper of "get the browser clean" was proposed.  I claim this is hard to get right.  Dtownsend does not.  Perhaps he can attempt it, and share it into the test/utils?

Comment 6

[Dave Townsend [:mossop]]

(In reply to Gregg Lind (User Research - Test Pilot) from comment #5)
> In both our proposals, a helper of "get the browser clean" was proposed.  I
> claim this is hard to get right.  Dtownsend does not.  Perhaps he can
> attempt it, and share it into the test/utils?

Sorry but I don't have time to work on this. Zombie did just land code in bug 1032514 that returns the tabs to the clean state so you can probably work from that.

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

I don't think we should untangle the two in our test framework, but another test framework could do that and we should support other test frameworks.

Do you think this is a good way to handle this Irakli?

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

I've made bug 1062756 about creating an "test" package key to define the entry point used when running tests, which would allow third party test frameworks to be used.

I think we should wontfix this, or mark it as a dup for bug 1062756, or make this bug more explicitly about making a cleanup helper function.

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Gregg I think you make some valid points. That being said more than one window or tab open has being a major source of oranges for us & while this solution is not ideal it helped us a lot in practice.

I think it would be a good compromise to either:

1. Add a decorator function that we could decorate tests to say that they may not clean up after themselves & that's fine.
2. Add an optional CLI flag to cfx / jpm that would run test runner in a mode where not cleaning up would be acceptable.

I'd be fine with taking either of these changes.


In long term though I totally agree with what Erik said & that would be my preferred evolution. Users should have a freedom to choose a different test framework which subsequently could make different choices about what is considered failure.

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

To be clear, I do think that two compromises proposed above are, well compromises, that can be an quick solutions to make everyone happy. Our general direction would definitely be choose your own test framework, it's just may take while to get there.

Comment 11

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

1.  I don't really want to choose my own framework!  I want Editorializing!  That the test framework is already set up is one of the huge advantages that jetpack has!  I know I can write my own, or set up my own, but *that is another distraction from actually writing the addon*.

2.  I scratched my own itch, to see how hard this is.  Answer:  It took me maybe 4 hours (all of which detracted from my actual work task, which is writing an addon that DOESNT CARE ABOUT CLEAN UI BETWEEN RUNS).  I could have done it less well, but both window close and tab close are async tasks, so I wanted to make sure they promised out / resolved after all close callbacks are complete.

3.  Code for utility-fn (with tests!):  https://gist.github.com/gregglind/62d8ee2bd517a408c77d

4.  I don't know how many people writes tests, so maybe this is a weird tangent anyway!  I know that I write tons.  I *can understand* the jetpack test suite (unlike in firefox core :) ).  In Test Pilot, it's hard enough not to hose people's systems, so anything that makes it less likely is something I want to use :).  Make testing awesome for me, please :)

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1617

Comment 13

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/77af704836b2f4934cabfc17201615a772bda0b3
Bug 1048427 adding a `cleanUI` util function in sdk/test/utils

https://github.com/mozilla/addon-sdk/commit/ae83c0f88923cdddde1e3724f9199b33e399a852
Merge pull request #1617 from erikvold/1048427

Bug 1048427 adding a `cleanUI` util function in sdk/test/utils r=@jsantell

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

We've added a helper function now to clean the UI as expected, hope that helps.  Also we plan to make it possible to use a custom main path when running tests in bug 1113022.