Closed Bug 1136892 Opened 6 years ago Closed 6 years ago

Split add-on manager xpcshell and mochitest-browser tests into their own test suites

Categories

(Testing :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mossop, Assigned: jgriffin)

References

Details

Attachments

(2 files, 1 obsolete file)

The tentative plan for bug 1133838 involves us running add-on manager tests on the non-branded beta and release builds instead of the real beta and release builds. This probably means moving these tests into their own test suites. We have both xpcshell and mochitest-browser tests. There is also a single mochitest test that uses an XPI but it might make more sense to just sign that XPI rather than splitting that off into its own suite.
Blocks: 1149656
Component: General Automation → General
Product: Release Engineering → Testing
QA Contact: catlee
Version: other → Trunk
There are also some addons tests in mochitest-chrome and a single addon test (dom/apps/tests/test_app_addons.html) in mochitest-plain; would we want to split these out as well?
The number of addon tests (at least those with 'addon' in the path) for xpcshell is pretty small:

toolkit/mozapps/extensions/test/xpcshell/test_no_addons.js
services/sync/tests/unit/test_addons_engine.js
services/sync/tests/unit/test_addons_store.js
services/healthreport/tests/xpcshell/test_provider_addons.js
services/sync/tests/unit/test_addons_reconciler.js
services/sync/tests/unit/test_addons_tracker.js

Ideally, we'd only split this out on the 'allow-unsigned-addons' build and just run them inside the main xpcshell job on the other build.
Assignee: nobody → jgriffin
Status: NEW → ASSIGNED
We may want to convert this to a subsuite, depending on the timing of getting signed addons available to the tests, but we can start with just a tag.
Attachment #8635560 - Flags: review?(cmanchester)
(In reply to Jonathan Griffin (:jgriffin) from comment #1)
> There are also some addons tests in mochitest-chrome and a single addon test
> (dom/apps/tests/test_app_addons.html) in mochitest-plain; would we want to
> split these out as well?

I'd argue no, for a few tests like that we can probably just check signed versions into the tree.

(In reply to Jonathan Griffin (:jgriffin) from comment #2)
> The number of addon tests (at least those with 'addon' in the path) for
> xpcshell is pretty small:

This really isn't a good way to check whether a test uses an add-on. Almost every single test in toolkit/mozapps/extensions/test/xpcshell uses one or more add-ons, some generated on the fly by JS code, some generated at build time from toolkit/mozapps/extensions/test/addons, some checked in under 
toolkit/mozapps/extensions/test/xpcshell/data. Likewise toolkit/mozapps/extensions/test/xpinstall almost all use add-ons.
> This really isn't a good way to check whether a test uses an add-on.

Oh I agree.  I was approaching this from the angle of, "what are the smallest set of tests that we want to run on the unsigned-addons-allowed build", once we have both of them running.

What is the actual goal here? To identify all of the tests which use addons that will need build-time signing?
Comment on attachment 8635560 [details] [diff] [review]
Create an xpcshell-addons tag for running addon-specific xpcshell tests,

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

::: services/healthreport/tests/xpcshell/xpcshell.ini
@@ +7,5 @@
>  [test_profile.js]
>  [test_healthreporter.js]
>  [test_provider_addons.js]
>  skip-if = buildapp == 'mulet'
> +tags = addons

Adding a tag doesn't exclude the test when running without a tag specified, I think we need to do something like mochitest does with subsuites.
Attachment #8635560 - Flags: review?(cmanchester)
That was actually my intention; one one of the builds, we (may want to) only run the addons tests; on the other build, we'll run them all without splitting apart the addons tests.

But I'll wait for a response to comment #5 before changing or reflagging for review.
>> What is the actual goal here? To identify all of the tests which use addons that will need build-time signing?

Yes.  We need to identify the tests that we can run on the builds where add on signing is enabled versus the tests that can run where addon signing is not enforced so we can invoke them separately.
So, most of our existing tests require addons incidentally (even if they're not explicitly testing addons), including _all_ of mochitest and reftest and their derivatives.  The only harness we could create this definitive split for is xpcshell.

If what we're doing is finding all the tests that validate addon handling so we can invoke only those, separately, on the unsigned-addons-allowed build, then we can do that.  What we can't do is split out tests so we can exclude addon-related tests from the signed-addons-required build until we have a signing mechanism in place, since nearly all of them required addons that will have to be signed.

Dave, can you clarify?
Flags: needinfo?(dtownsend)
I'm not sure what you're asking me for here. Looks like your statement is accurate.
Flags: needinfo?(dtownsend)
Take 2; this grabs all the addon-related xpcshell tests I could identify, which should be enough for testing the unsigned-addons-allowed build, even if I missed a couple.  I don't want to use subsuite atm, since the goal is to leave the jobs as-is for the signed-addons-required build, but allow just the addon-related tests to be signed on the other.
Attachment #8649011 - Flags: review?(ahalberstadt)
Attachment #8635560 - Attachment is obsolete: true
Comment on attachment 8649011 [details] [diff] [review]
Create an xpcshell-addons tag for running addon-specific xpcshell tests,

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

Whoops, wrong reviewer.
Attachment #8649011 - Flags: review?(ahalberstadt) → review?(cmanchester)
Attachment #8649011 - Flags: review?(cmanchester) → review+
Keywords: leave-open
You need commas before your added lines in linux_unittest.py and mac_unittest.py and win_unittest.py to make the syntax valid, right?
(In reply to Wes Kocher (:KWierso) from comment #15)
> You need commas before your added lines in linux_unittest.py and
> mac_unittest.py and win_unittest.py to make the syntax valid, right?

Indeed, sorry for the bustage.
Flags: needinfo?(jgriffin)
Same thing, for mochitest-browser-chrome.  This time, with green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f62aacc4111
Attachment #8651110 - Flags: review?(ahalberstadt)
Comment on attachment 8651110 [details] [diff] [review]
Create a browser-chrome-addons tag for running addon-specific browser-chrome tests,

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

I think you meant to flag chmanchester again, but this looks good to me :).
Attachment #8651110 - Flags: review?(ahalberstadt) → review+
(In reply to Andrew Halberstadt [:ahal] from comment #20)
> Comment on attachment 8651110 [details] [diff] [review]
> Create a browser-chrome-addons tag for running addon-specific browser-chrome
> tests,
> 
> Review of attachment 8651110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you meant to flag chmanchester again, but this looks good to me :).

He was PTO so I r?'d you.  Thanks!
Keywords: leave-open
This is complete, at least as far as we understand the requirements.  I'll leave it open until we can verify this is enough, or understand what additional work is needed.
I'm going to call this done for now; will open a new bug if more work is needed.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.