1051886 - Split webgl tests into a separate mochitest-gl suite on desktop

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

The webgl conformance tests have caused ongoing stability issues for years now. Running as part of mochitest-2 on desktop, we've reached a point where on Linux, it doesn't meet default visibility requirements due to the high failure rate due mostly to bug 777574. We've attempted to work around the issues in the past by selectively disabling sub-tests, but that only pushed the failures to other tests.

I don't want to hide mochitest-2 by default due to the high collateral damage that would cause with all the other tests that happen to run in that suite. Therefore, I would like to split out the webgl tests into their own suite like bug 865443 did for Android, so they can be hidden by default and left to the WebGL maintainers to monitor until they can be sufficiently stabilized for meeting the job visibility requirements.

Comment 1

[Ed Morley [:emorley]]

+1

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Sorry, comment 0 should have said mochitest-1, not mochitest-2.

Comment 3

[Kim Moir [:kmoir] ET]

:jmaher: what needs to change on the test manifest side of things?

Comment 4

[Kelsey Gilbert [:jgilbert]]

+1 on splitting out a mochitest-gl.

I could accept hiding mochitest-gl, but we *must* run it. We cannot afford to disable it.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

We can't afford to hide it either, actually, as we do care about these tests not only running but actually passing. If the intermittent rate is so high that it's a burden to tree sheriffs and they need to hide it, that means that we've been relying on sheriff's goodwill and energy too much and we should fix the tests; that does *not* mean that we didn't care about the test coverage that that gave us.

Comment 6

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #5)
> We can't afford to hide it either, actually, as we do care about these tests
> not only running but actually passing. If the intermittent rate is so high
> that it's a burden to tree sheriffs and they need to hide it, that means
> that we've been relying on sheriff's goodwill and energy too much and we
> should fix the tests; that does *not* mean that we didn't care about the
> test coverage that that gave us.

I'm working on fixing it, but I can handle the tests being hidden for a week while I fix this.
Maybe it's optimistic, but I think splitting the single mega-test up will make things more manageable.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Now that bug 777574 is fixed, I assume that we're no longer talking of having to hide WebGL tests.

Splitting them into a separate suite would still be nice, though.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

We are talking about:
dom/canvas/test/webgl-conformance/mochitest.ini

this is a standalone file and we could mark it as a subsuite (as we did in bug 984930 for mochitest-devtools).  As a subsuite it won't run by default and our builder for mochitest-gl could just add --subsuite=webgl.  As you can see in bug 984930 there was a lot of stuff that had to happen (some was renaming and some was changing the number of chunks, both of which wouldn't need to happen here)

Comment 9

[Kim Moir [:kmoir] ET]

Attachment: bug1051886mh.patch

tested in staging on ubuntu32 slave

Comment 10

[Kim Moir [:kmoir] ET]

Attachment: bug1051886bbconfigs.patch

enable on cedar to start

Comment 11

[Jordan Lund (:jlund)]

Comment on attachment 8481490 [details] [diff] [review]
bug1051886mh.patch

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

IIUC, this should pretty much do it :)

r+ if you confirm 1) mac/win is different than linux and all args are needed or 2) you remove/add the un-needed args :D

::: configs/unittests/mac_unittest.py
@@ +57,5 @@
>          "browser-chrome-1": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=1"],
>          "browser-chrome-2": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=2"],
>          "browser-chrome-3": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=3"],
>          "browser-chrome-chunked": ["--browser-chrome", "--chunk-by-dir=5"],
> +        "mochitest-webgl": ["--mochitest-webgl --subsuite=webgl"],

does runtests.py need --mochitest-webgl ?

If it does, do we need to add it to linux_unittest.py?

Do spaces work within a string here? Since this is a list, I suspect we use this in run_command() via a list of strings over one string with spaces.

::: configs/unittests/win_unittest.py
@@ +67,5 @@
>          "browser-chrome-1": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=1"],
>          "browser-chrome-2": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=2"],
>          "browser-chrome-3": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=3"],
>          "browser-chrome-chunked": ["--browser-chrome", "--chunk-by-dir=5"],
> +        "mochitest-webgl": ["--mochitest-webgl --subsuite=webgl"],

same Q as mac_unittest.py

Comment 12

[Jordan Lund (:jlund)]

Comment on attachment 8481492 [details] [diff] [review]
bug1051886bbconfigs.patch

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

lgtm! I'm guessing 1) we are not running this against debug yet? and 2) we will continue running webgl tests in mochi-1 (even on cedar) for now?

Comment 13

[Kelsey Gilbert [:jgilbert]]

(In reply to Jordan Lund (:jlund) from comment #12)
> Comment on attachment 8481492 [details] [diff] [review]
> bug1051886bbconfigs.patch
> 
> Review of attachment 8481492 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm! I'm guessing 1) we are not running this against debug yet? and 2) we
> will continue running webgl tests in mochi-1 (even on cedar) for now?

We run on debug on Desktop platforms. This cannot be regressed.

Comment 14

[Kim Moir [:kmoir] ET]

:jlund thanks for the reviews, this revealed a problem with my patches that I have been trying to address today.  I thought my tests were working but looking at the logs more closely, they were not working as expected.

:jmaher: Instead of using subsuite (which I can't seem to get to work even though I have been modifying runtests.py) could I just use manifest instead (tests/dom/canvas/test/webgl-mochitest/mochitest.ini)?

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

:kmoir, yes, we could just point at the ini file.  We should chat in IRC or outside of this bug as to why you are not getting subsuite to work.

My only concern would be- will we still be running the webgl tests in mochitest-2

Comment 16

[Kim Moir [:kmoir] ET]

Attachment: bug1051886bbconfigs-2.patch

added debug

Comment 17

[Kim Moir [:kmoir] ET]

Attachment: bug1051886mh-2.patch

changed from using a subsuite to a manifest

Comment 18

[Jordan Lund (:jlund)]

Comment on attachment 8483581 [details] [diff] [review]
bug1051886mh-2.patch

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

if these inputs to runtests wfy it wfm :)

Comment 19

[Jordan Lund (:jlund)]

Comment on attachment 8483580 [details] [diff] [review]
bug1051886bbconfigs-2.patch

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

lgtm :)

do we need a tbpl patch?

Comment 20

[Chris Cooper [:coop] (he/him)]

Merged to production, and deployed.

Comment 21

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8483581 [details] [diff] [review]
bug1051886mh-2.patch

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

::: configs/unittests/win_unittest.py
@@ +67,5 @@
>          "browser-chrome-1": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=1"],
>          "browser-chrome-2": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=2"],
>          "browser-chrome-3": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=3"],
>          "browser-chrome-chunked": ["--browser-chrome", "--chunk-by-dir=5"],
> +        "mochitest-webgl": ["--manifest=tests/mochitest/tests/dom/canvas/test/webgl-mochitest/mochitest.ini"],

This is just the webgl-mochitest dir tests. The main body of webgl tests is in 'webgl-conformance/_mochitest.ini'

Comment 22

[Kelsey Gilbert [:jgilbert]]

Also, when should this be taking effect? I don't see any non-numbered mochitests running on desktop runs on central or inbound, even in self-serve API.

Comment 23

[Kim Moir [:kmoir] ET]

It was just enabled on cedar today, I'll fix the .ini file in mozharness, sorry about that.

Comment 24

[Kim Moir [:kmoir] ET]

Attachment: bug1051886fixmh.patch

correct manifest -bash-4.1$ unzip -l firefox-34.0a1.en-US.linux-i686.tests.zip | grep webgl-conformance | grep webgl-conformance | grep mochitest.ini
      697  08-28-2014 03:35   mochitest/tests/dom/canvas/test/webgl-conformance/mochitest.ini


tested in staging green tests
http://dev-master1.srv.releng.scl3.mozilla.com:8036/builders/Ubuntu%20VM%2012.04%20cedar%20opt%20test%20mochitest-webgl/builds/29/steps/run_script/logs/stdio

Comment 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8485335 [details] [diff] [review]
bug1051886fixmh.patch

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

::: configs/unittests/win_unittest.py
@@ +67,5 @@
>          "browser-chrome-1": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=1"],
>          "browser-chrome-2": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=2"],
>          "browser-chrome-3": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=3"],
>          "browser-chrome-chunked": ["--browser-chrome", "--chunk-by-dir=5"],
> +        "mochitest-webgl": ["--manifest=tests/mochitest/tests/dom/canvas/test/webgl-conformance/mochitest.ini"],

This is no longer the correct mochitest manifest. Please use '_mochitest.ini'. I haven't landed the deletion of the old one yet.

Also, ideally, mochitest-webgl would cover dom/canvas/test/webgl-*/, not just one of webgl-conformance or webgl-mochitest.

Comment 26

[Kim Moir [:kmoir] ET]

Okay, I didn't see _mochitest.ini in the tests zip I was using for testing that's why I used mochitest.ini.  I'll fix it.

Comment 27

[Kim Moir [:kmoir] ET]

regarding

>Also, ideally, mochitest-webgl would cover dom/canvas/test/webgl-*/, not just one of webgl-conformance or >webgl-mochitest.

Currently you can only specify one manifest per test suite in buildbot so I have to either refactor how this works or look at subsuites again. Investigating.

Comment 28

[Kelsey Gilbert [:jgilbert]]

(In reply to Kim Moir [:kmoir] from comment #27)
> regarding
> 
> >Also, ideally, mochitest-webgl would cover dom/canvas/test/webgl-*/, not just one of webgl-conformance or >webgl-mochitest.
> 
> Currently you can only specify one manifest per test suite in buildbot so I
> have to either refactor how this works or look at subsuites again.
> Investigating.

Can I give you a new dom/canvas/test/mochitest-webgl.ini which brings both of these in, and use that mochitest manifest for the subsuite?

Comment 29

[Kim Moir [:kmoir] ET]

Yes that would be great. thanks!

Comment 30

[Kelsey Gilbert [:jgilbert]]

(In reply to Kim Moir [:kmoir] from comment #29)
> Yes that would be great. thanks!

Bug 1064432 is this, and should land shortly.

Comment 31

[Jordan Lund (:jlund)]

Comment on attachment 8485335 [details] [diff] [review]
bug1051886fixmh.patch

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

Clearing my queue. Feel free to ask for review on the next patch. If it is just a manifest path change, maybe jgilbert would be better suited :)

Comment 32

[Kim Moir [:kmoir] ET]

Attachment: rename mochitest-webgl to mochitest-gl to match Android as per RyanVM

Comment 33

[Kim Moir [:kmoir] ET]

Attachment: glrenamemh.patch

same change in mh 

tested in staging

Comment 34

[Jordan Lund (:jlund)]

Comment on attachment 8493161 [details] [diff] [review]
glrenamemh.patch

lgtm

Comment 35

[Jordan Lund (:jlund)]

Comment on attachment 8493159 [details] [diff] [review]
rename mochitest-webgl to mochitest-gl to match Android as per RyanVM

lgtm

Comment 36

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8493159 [details] [diff] [review]
rename mochitest-webgl to mochitest-gl to match Android as per RyanVM

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

::: mozilla-tests/config.py
@@ +940,1 @@
>                      'config_files': ["unittests/linux_unittest.py"],

The surrounding blocks here use win_unittest. Should this be linux_unittest anyways?

@@ +1313,1 @@
>                      'config_files': ["unittests/linux_unittest.py"],

Same as above, but surrounded by mac_unittest.

Comment 37

[Kelsey Gilbert [:jgilbert]]

Also, can we get mochitest-gl on B2G?
With these patches, on Ceder I see mochitest-gl on windows/mac/linux/android, but not b2g.

The Linux runs (at least) appear to be running non-webgl tests in mochitest-gl:
> 5180 INFO TEST-UNEXPECTED-ERROR | /tests/dom/ipc/tests/test_CrashService_crash.html | This test did not leave any crash dumps behind, but we were expecting some!
> 5181 INFO TEST-UNEXPECTED-ERROR | /tests/dom/ipc/tests/test_CrashService_crash.html | This test left crash dumps behind, but we weren't expecting it to!
> 5183 INFO TEST-UNEXPECTED-FAIL | /tests/dom/ipc/tests/test_CrashService_crash.html | undefined assertion name - Result logged after SimpleTest.finish()
> 5184 INFO TEST-UNEXPECTED-FAIL | /tests/dom/ipc/tests/test_CrashService_crash.html | undefined assertion name - Result logged after SimpleTest.finish()
> 5312 INFO TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/ipc/test_ipc.html | test_dataChannel_basicAudioVideo.html - This test left crash dumps behind, but we weren't expecting it to!

Comment 38

[Pete Moore [:pmoore][:pete]]

Something here landed in production today: https://wiki.mozilla.org/ReleaseEngineering/Maintenance#Reconfigs_.2F_Deployments

Comment 39

[Kelsey Gilbert [:jgilbert]]

The mochitest-gl jobs on Cedar are definitely running non-webgl mochitests. Can someone fix this?

Comment 40

[Joel Maher ( :jmaher ) (UTC -8)]

the webgl jobs are referencing a manifest which doesn't exist.  here is the command we execute:
/builds/slave/test/build/venv/bin/python -u /builds/slave/test/build/tests/mochitest/runtests.py --appname=/builds/slave/test/build/application/firefox/firefox --utility-path=tests/bin --extra-profile-file=tests/bin/plugins --symbols-path=https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/cedar-linux/1413220385/firefox-35.0a1.en-US.linux-i686.crashreporter-symbols.zip --certificate-path=tests/certs --autorun --close-when-done --console-level=INFO --setpref=webgl.force-enabled=true --quiet --log-raw=/builds/slave/test/build/blobber_upload_dir/raw_structured_logs.log --use-test-media-devices --manifest=tests/mochitest/tests/dom/canvas/test/webgl-mochitest/mochitest.ini

If you look, we are expecting this file: tests/mochitest/tests/dom/canvas/test/webgl-mochitest/mochitest.ini

but what we really want is: tests/mochitest/tests/dom/canvas/test/webgl-mochitest.ini


so we need to modify the script to point to the correct manifest, this patch has pointers of what files to change:
https://bug1051886.bugzilla.mozilla.org/attachment.cgi?id=8493161

this is in the mozharness repo:
http://hg.mozilla.org/build/mozharness/file/d6ef7a71bdce/configs/unittests

Comment 41

[Kelsey Gilbert [:jgilbert]]

Ah, ok. That's the wrong manifest. This is the correct manifest:
http://mxr.mozilla.org/mozilla-central/source/dom/canvas/test/mochitest-subsuite-webgl.ini

Comment 42

[Kim Moir [:kmoir] ET]

Attachment: bug1051886subsuite.patch

Sorry for the delay in looking at this bug, I have been at a releng workshop all week

Comment 43

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8505777 [details] [diff] [review]
bug1051886subsuite.patch

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

Wrong directory. I had to move the manifest down a directory.

::: configs/unittests/win_unittest.py
@@ +67,5 @@
>          "browser-chrome-1": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=1"],
>          "browser-chrome-2": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=2"],
>          "browser-chrome-3": ["--browser-chrome", "--chunk-by-dir=5", "--total-chunks=3", "--this-chunk=3"],
>          "browser-chrome-chunked": ["--browser-chrome", "--chunk-by-dir=5"],
> +        "mochitest-gl": ["--manifest=tests/mochitest/tests/dom/canvas/test/webgl-mochitest/mochitest-subsuite-webgl.ini"],

tests/mochitest/tests/dom/canvas/test/mochitest-subsuite-webgl.ini

Comment 44

[Kim Moir [:kmoir] ET]

Attachment: bug1051886subsuite-2.patch

Comment 45

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8505880 [details] [diff] [review]
bug1051886subsuite-2.patch

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

Looks good, thanks!

Comment 46

[Pete Moore]

In production: https://hg.mozilla.org/build/mozharness/rev/132b59dcbe39

Comment 47

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8493159 [details] [diff] [review]
rename mochitest-webgl to mochitest-gl to match Android as per RyanVM

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

mochitest-gl is not running properly on mac or windows, and I bet this is why.

Comment 48

[Kim Moir [:kmoir] ET]

Attachment: bug1051886fixtypo.patch

Comment 49

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/buildbot-configs/rev/227fc1edc191

Comment 50

[Kelsey Gilbert [:jgilbert]]

The newest cedar run looks like it's running mochitest-gl satisfactorily on all desktop platforms:
https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=8d185e7c78ee

It is still running the same tests in mochitest-2, so we do need to disable those.

When can we see this on inbound/central/try?

Comment 51

[Kelsey Gilbert [:jgilbert]]

Thanks for the work, too! :)

Comment 52

[Kim Moir [:kmoir] ET]

I'll write a patch to enable on inbound/central/try

Comment 53

[Kim Moir [:kmoir] ET]

I have a patch to enable them on where gecko is at least 36, which enables them on inbound/central/try + associated project branches.  Usually we enable tests to ride the trains vs just enabling them on specific branches - is this okay for you?

Comment 54

[Kelsey Gilbert [:jgilbert]]

(In reply to Kim Moir [:kmoir] from comment #53)
> I have a patch to enable them on where gecko is at least 36, which enables
> them on inbound/central/try + associated project branches.  Usually we
> enable tests to ride the trains vs just enabling them on specific branches -
> is this okay for you?

Yes, thanks!

Comment 55

[Kim Moir [:kmoir] ET]

Attachment: bug1051886ridetrains.patch

tested in staging, will attach builder diff

Comment 56

[Kim Moir [:kmoir] ET]

Attachment: bug1051886builder.diff

builder diff

Comment 57

[Jordan Lund (:jlund)]

Comment on attachment 8511052 [details] [diff] [review]
bug1051886ridetrains.patch

lgtm.

sanity check, does https://bugzilla.mozilla.org/show_bug.cgi?id=1051886#c50 mean releng needs to disable mochitest chunk 2 or ateam needs to disable specific tests within mochitest chunk 2?

Comment 58

[Kim Moir [:kmoir] ET]

I assumed that  https://bugzilla.mozilla.org/show_bug.cgi?id=1051886#c50 meant ateam disabling specific tests in the manifest.  :jgilbert, can you confirm?

Comment 59

[Kelsey Gilbert [:jgilbert]]

(In reply to Kim Moir [:kmoir] from comment #58)
> I assumed that  https://bugzilla.mozilla.org/show_bug.cgi?id=1051886#c50
> meant ateam disabling specific tests in the manifest.  :jgilbert, can you
> confirm?

Yep, just those specific tests. I'm not sure how the list that is chunked is created. Maybe if we remove the entries from moz.build?

Comment 60

[Kim Moir [:kmoir] ET]

The list of chunked tests is created by mozharness given the manifest for the tests.  If the gl tests are in a new manifest, you should be able to comment them out in the original one, but please confirm.

Comment 61

[Kelsey Gilbert [:jgilbert]]

(In reply to Kim Moir [:kmoir] from comment #60)
> The list of chunked tests is created by mozharness given the manifest for
> the tests.  If the gl tests are in a new manifest, you should be able to
> comment them out in the original one, but please confirm.

Ok, I'll need to move files around then.

Comment 62

[Release Engineering SlaveAPI Service]

In production: https://hg.mozilla.org/build/buildbot-configs/rev/d04c32aec135

Comment 63

[Kim Moir [:kmoir] ET]

So the gl tests are green on those branches on treeherder so I think the only work left to do is to update the manifests to exclude those tests from running as part of m-2

Comment 64

[Kim Moir [:kmoir] ET]

jgilbert: could you update the manifests as you mentioned in comment #61?

Comment 65

[Kelsey Gilbert [:jgilbert]]

(In reply to Kim Moir [:kmoir] from comment #64)
> jgilbert: could you update the manifests as you mentioned in comment #61?

This approach has yielded bug 1141771.

I'm going to mark this fixed, since the issue in the bug summary has been fixed.

Work on not running the tests twice should continue in a different bug. I filed bug 1141778 for this.