Closed Bug 1194789 Opened 5 years ago Closed 5 years ago

Disable mochitest-ipcplugins and remove mochitest-ipcplugins code


(Testing :: General, defect)

Not set


(firefox43 fixed)

Tracking Status
firefox43 --- fixed


(Reporter: benjamin, Assigned: parkouss)



(2 files, 1 obsolete file)

The mochitest-ipcplugins code covers the case where plugins are configured to run in-process. In bug 1194780 I am removing that code path and so this test is no longer useful.

Let's disable mochitest-ipcplugins and remove its vestiges.

>>Mozilla Top Level Goal:
Supporting less code and making tests run faster?

>>Existing Bug:
No bug

It currently runs per-commit.

>>Data other than Pass/Fail:

>>Prototype Date:
Not provided

>>Production Date:
Not provided

>>Most Valuable Piece:
Not provided

>>Responsible Engineer:

Not provided

>>Other Teams/External Dependencies:
Not provided

>>Additional Info:
Not provided
I think I asked about this on IRC recently after seeing it come up in a bug. This should be fairly straightforward, mostly just code removal. With mozharness in-tree we probably don't need any buildbot changes at all (since it's lumped into mochitest-other, not run as a separate test job).
cc'ing Julien in case he wants to take a crack at it.
Yep, why not. :) Here is a copy paste of a conversation on irc that will help me:

23:36 <chmanchester> parkouss: mozharness calls it the "plugins" subsuite
23:36 <chmanchester> parkouss:
23:37 <parkouss> chmanchester: hm, I'm not following
23:37 <chmanchester> also
23:39 <RyanVM> hmm, I wonder why we don't run windows debug cpptests in production
23:39 <chmanchester> parkouss: ok. That buildbot link means we invoke with "--mochitest-suite chrome,a11y,plugins"
23:40 <chmanchester> parkouss: which gets picked up here:
23:41 <-- ahal ( a quitté (A TLS packet with unexpected length was received.)
23:41 <chmanchester> parkouss: and becomes a key for the config dict in that first link
23:41 <chmanchester> ("plugins" is the key)
23:42 <parkouss> chmanchester: ok - still not sure why you show me that ?
23:42 <chmanchester> parkouss: the bug you linked is about tearing out that subsuite
23:43 <parkouss> chmanchester: ahhh
23:43 <parkouss> hmm
23:43 <parkouss> right
23:43 <chmanchester> for completeness, it's also mentioned in :)
23:44 <parkouss> chmanchester: yep, I saw that one (well ahal pointed me that one)
23:44 <parkouss> chmanchester: cool, thanks
23:44 <chmanchester> parkouss: np
Attached patch 1194789.patch (obsolete) — Splinter Review
I think I removed the stuff related to mochitest-ipcplugins.

Also removed the dom.ipc* browser preferences of the mochitest plugins suite in testing/mozharness/configs/unittests/{mac,linux,win} but was not sure about that.
Assignee: jgriffin → j.parkouss
Attachment #8648674 - Flags: review?(jgriffin)
I just tested locally - ./mach test seems good, mochitest-ipcplugins is no more available and I can run another mochi test using that command.
Comment on attachment 8648674 [details] [diff] [review]

Review of attachment 8648674 [details] [diff] [review]:

We'll also need to turn this suite off in buildbot-configs, and do that before this patch lands.  I can make a patch for that.

::: testing/mozharness/configs/unittests/
@@ +75,5 @@
>          "mochitest-devtools-chrome-chunked": ["--browser-chrome", "--subsuite=devtools", "--chunk-by-runtime"],
>          "jetpack-package": ["--jetpack-package"],
>          "jetpack-addon": ["--jetpack-addon"],
>          "a11y": ["--a11y"],
> +        "plugins": ['--manifest=tests/dom/plugins/test/mochitest/mochitest.ini']

We can remove 'plugins' altogether; the plugin tests are already run (with no special prefs) in mochitest-plain.
Attachment #8648674 - Flags: review?(jgriffin) → review-
This was easier than I thought...
Attachment #8648857 - Flags: review?(ahalberstadt)
Assignee: j.parkouss → jgriffin
Sorry, didn't mean to reassign.
Assignee: jgriffin → j.parkouss
Attachment #8648857 - Flags: review?(ahalberstadt) → review+
Attached patch 1194789.patchSplinter Review
Fixed according to comments. :)

Maybe we can try/land both patches in one go ?
Attachment #8648674 - Attachment is obsolete: true
Attachment #8648868 - Flags: review?(jgriffin)
(In reply to Julien Pagès from comment #9)
> Created attachment 8648868 [details] [diff] [review]
> 1194789.patch
> Fixed according to comments. :)
> Maybe we can try/land both patches in one go ?

We can't, because the buildbot patch has to be pushed to production before the gecko patch lands, otherwise we'll be seeing red mochitest-other jobs in Treeherder!
Comment on attachment 8648868 [details] [diff] [review]

Review of attachment 8648868 [details] [diff] [review]:

Thanks, this looks good.  I'll wait to r+ it until it's safe to land, pending the deployment of the buildbot patch.
In production
Comment on attachment 8648868 [details] [diff] [review]

Review of attachment 8648868 [details] [diff] [review]:

Ok, should be safe to land!
Attachment #8648868 - Flags: review?(jgriffin) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.