838309 - Run test suite with private-browsing support off and then again with it on

Status: RESOLVED WONTFIX

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

Description

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

I'd like to have all of our tests run twice but only when using `cfx test` in the sdk for now, once with the default loader supporting pb, and another time when it is not.

We could get around doing this, but it would mean rewriting a lot of tests do use the loader module and manually run tests that could be affected in both pb modes (supported/not), which means identifying every test that could be affected.  I think we should still do this identification process and write better tests using the loader module, but this would be a good safety net to have in place I believe.

Comment 1

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

Alex do you think you'd have time for this in the next couple of weeks? I don't think I'll be able to tackle it in that time frame.  Also do you think it's necessary?

Comment 2

[Alexandre Poirot [:ochameau]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #1)
> next couple of weeks?
Yes.

> Also do you think it's necessary?

I'd not say we should run all tests twice, otherwise it will cost us too much time to run them before landing patches. Having an optional flag makes sense, then having dedicated test addons with the two flavors sounds like a good option.

See https://github.com/mozilla/addon-sdk/pull/772 for example of such test addon.

Comment 3

[Alexandre Poirot [:ochameau]]

I gave it a try by manually patching package.json at SDK root folder, by adding that:
  "permissions": {
    "private-browsing": true
  }

And all tests are passing except tests that explicitely check that private browing flag is turned off.
So, Erik, if you never tried that and that tests are just passing without even having to do anything special, I'd say running all tests twice don't worth it.

Comment 4

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

So I think this is necessary now.  It would have been easy to spot bug 853336 if this bug were resolved.

Comment 5

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

any chance you can take this one Alex?

Comment 6

[Alexandre Poirot [:ochameau]]

I'm still suspicious about the benefit of doing that, especially regarding its cost.
It will more or less double test execution time!!
For me, we were missing tests.
I applied same trick I used in comment 3 to run tests with private browsing permission and it now fails with:

Error: Error: The panel module cannot be used with per-window private browsing a
t the moment, see Bug 816257
 Traceback (most recent call last):
  File "resource://extensions.modules.09069311-548e-4990-bf39-ca56903f0871-at-je
tpack.commonjs.path.tests/test-panel.js", line 856, in
    require("sdk/panel");
  File "resource://extensions.modules.09069311-548e-4990-bf39-ca56903f0871-at-je
tpack.commonjs.path/sdk/loader/cuddlefish.js", line 127, in options<.load
    result = load(loader, module);
  File "resource://extensions.modules.09069311-548e-4990-bf39-ca56903f0871-at-je
tpack.commonjs.path/sdk/panel.js", line 36, in
    throw Error('The panel module cannot be used with per-window private browsin
g at the moment, see Bug 816257');
0 of 1 tests passed.

I dislike having tests that do a lot of conditional depending on the environnement we run the test into. I find it more maintainable to have test addons that run on one given environnement so that we end up with readable tests.

Having said that, I can easily add a cfx flag to optionally run sdk tests with private browsing permission set. But really, we should balance the cost vs benefit of running tests twice!

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 915

Here is a patch to add such option.

Comment 8

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

I agree that testaddons were all that we needed.