1401207 - Allows DAMP to run only a specific set of tests

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

For now, DAMP always run everything. But it takes a lot of time whereas in most cases you only care to run a couple of tests locally.

We should be able to run only a specific list of these DAMP tests.
May be just a regexp that matches the test string displayed in perfherder subtests?

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1401207 - Refactor DAMP to more easily filter subtests.

Review commit: https://reviewboard.mozilla.org/r/188554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188554/

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8917604 [details]
Bug 1401207 - Refactor DAMP to more easily filter subtests.

Joel, here is the patch I was refering to.
What do you think about the changes in testing/talos/talos/*.py?
More work will be necessary in damp.js in order to ease filtering the tests.

Comment 3

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

Comment on attachment 8917604 [details]
Bug 1401207 - Refactor DAMP to more easily filter subtests.

https://reviewboard.mozilla.org/r/188554/#review194184

this is really simple to achieve a lot of good functionality.  I believe the best route here would be passing a value in via a preference instead of a cli parameter- that helps make it more generic for other tests to participate.

One other topic related to this is defining what is a subtest.  We have some tests which load a series of webpages:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/perf-reftest-singletons/perf_reftest_singletons.manifest

then there is damp which loads one page and does many tests inside that one page:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/devtools/damp.manifest

we have a mix of tests which go either way.  If we were to make something more generic for subtests, we would need to define all subtests in a common manifest instead of inside of the test files themselves- this will be hard for larger benchmarks (like canvasmark, speedometer).

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #3)
> Comment on attachment 8917604 [details]
> Bug 1401207 - Allow DAMP to run only a subset of tests.
> 
> https://reviewboard.mozilla.org/r/188554/#review194184
> 
> this is really simple to achieve a lot of good functionality.  I believe the
> best route here would be passing a value in via a preference instead of a
> cli parameter- that helps make it more generic for other tests to
> participate.

Ok.
Is there a limit on pref size? Because if we introduce "talos.ini" or "damp.ini" support, we would use that same filtering argument to pass the full list of all subtests to execute, and so, that may be a long string!

> One other topic related to this is defining what is a subtest.  We have some
> tests which load a series of webpages:
> https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/
> perf-reftest-singletons/perf_reftest_singletons.manifest
> 
> then there is damp which loads one page and does many tests inside that one
> page:
> https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/
> devtools/damp.manifest

So first thing to acknowledge is that DAMP integration into pageloader is a hack.
It just happened to be easier to build it like that, but the tests aren't so much oriented against loading webpages. Yes, we do load two pages, but some tests in DAMP do not depend on having a page loaded.
At the end I would not consider DAMP as a pageloader test and I think we would benefit from making it its own thing and remove most of pageloader things we do not use at all.

Having said that your comment is very relevant and as I said in my previous comment,
a significant work is required in DAMP to better define subtests.
Ideally we would have one file per test. I just don't know exactly how to get to that setup yet.
I would like to do a first iteration and still keep everything in damp.js, but have a big dictionnary indexed by sub test name. Then filtering should be much easier to implement and understand when you read damp.js.
After that, we can followup on having multiple files and filter by file name instead.
And only after all that, we can start thinking about having subtests manifest in *.ini files.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1401207 - Implement ./mach talos --subtests to allow filtering DAMP tests.

Review commit: https://reviewboard.mozilla.org/r/190230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190230/

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8917604 [details]
Bug 1401207 - Refactor DAMP to more easily filter subtests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188554/diff/1-2/

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8919381 [details]
Bug 1401207 - Implement ./mach talos --subtests to allow filtering DAMP tests.

Would such patch work for you?
See the other one, where I refactor DAMP in order to better define all subtests.

I think this is now is a decent shape if you consider only DAMP.

Followup would be to split damp.js in multiple files and have the --subtests filter by file name instead of test name (file name will reflect current test name). I don't want to block on that as subtest filtering is more important and awaited by my team.

Comment 8

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

Comment on attachment 8919381 [details]
Bug 1401207 - Implement ./mach talos --subtests to allow filtering DAMP tests.

https://reviewboard.mozilla.org/r/190230/#review195540

this would be easier to apply to other tests, basically we can pass this into pageloader and work with other tests that have pages defined in a .manifest file.  We can also continue to pass to the benchmark and filter there as you are doing.

I would like to make sure it is easy to find the list of subtests, and it is well tested for invalid names, single name, multiple names.

::: testing/talos/talos/cmdline.py:72
(Diff revision 1)
>      add_arg('-a', '--activeTests',
>              help="List of tests to run, separated by ':' (ex. damp:cart)")
>      add_arg('--suite',
>              help="Suite to use (instead of --activeTests)")
> +    add_arg('--subtests',
> +            help="Name of the subtest(s) to run (works only on DAMP)")

is this a list value, or something which we can specify over and over again.  I am curious how to specify a few subtests.

Comment 9

[Alexandre Poirot [:ochameau]]

(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #8)
> Comment on attachment 8919381 [details]
> Bug 1401207 - Implement ./mach talos --subtests to allow filtering DAMP
> tests.
> 
> https://reviewboard.mozilla.org/r/190230/#review195540
> 
> this would be easier to apply to other tests, basically we can pass this
> into pageloader and work with other tests that have pages defined in a
> .manifest file.  We can also continue to pass to the benchmark and filter
> there as you are doing.

Would you have pointer to where such filtering should be added?
(one example with .manifest file and another one with a benchmark)
Just to see how that would fit and where.

> I would like to make sure it is easy to find the list of subtests, and it is
> well tested for invalid names, single name, multiple names.

In the current patch, everything is defined by damp.js implementation.
It is currently very naive: testName.includes(filterString)

> > +    add_arg('--subtests',
> > +            help="Name of the subtest(s) to run (works only on DAMP)")
> 
> is this a list value, or something which we can specify over and over again.
> I am curious how to specify a few subtests.

So, right now it is only a "substring filter value". I could support more pattern.

Note that I have followups in mind where actual filtering would change:
1/ I would like to have one file per subtest
  => subtests would be filtered by file name instead
2/ then, I would like to have "talos.ini" files (like xpcshell.ini, mochitest.ini, ...)
  => at that point the filter would work like ./mach mochitest FOO or ./mach test FOO
  => it would no longer be done by talos, but by mozbuild.testing.TestResolver
3/ finally, ./mach test would work for talos, or at least DAMP tests.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8917604 [details]
Bug 1401207 - Refactor DAMP to more easily filter subtests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188554/diff/2-3/

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8919381 [details]
Bug 1401207 - Implement ./mach talos --subtests to allow filtering DAMP tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190230/diff/1-2/

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8917604 [details]
Bug 1401207 - Refactor DAMP to more easily filter subtests.

https://reviewboard.mozilla.org/r/188554/#review195992

Thanks, looks good to me

Comment 13

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

Comment on attachment 8919381 [details]
Bug 1401207 - Implement ./mach talos --subtests to allow filtering DAMP tests.

https://reviewboard.mozilla.org/r/190230/#review196028

thanks for getting this in.  Hopefully this is the start of many great ways to use talos!

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failures in testing/talos/talos/tests/devtools/addon/content/damp.js:

https://hg.mozilla.org/integration/autoland/rev/404101537948219eafd5f6729615a5d51e4cb5f8
https://hg.mozilla.org/integration/autoland/rev/278cb714534f3aed4c024f7f561b1ea45c8f124b

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f42923db714b07f652d7e66913160824862784af&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=137988099&repo=autoland

 TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/devtools/addon/content/damp.js:494:5 | Expected space(s) after "for". (keyword-spacing)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/devtools/addon/content/damp.js:680:5 | provide a default value instead of using a try/catch block (mozilla/use-default-preference-values)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/devtools/addon/content/damp.js:682:7 | Expected space(s) after "catch". (keyword-spacing)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/talos/talos/tests/devtools/addon/content/damp.js:685:7 | Expected space(s) after "for". (keyword-spacing)

Comment 15

[Alexandre Poirot [:ochameau]]

Comment on attachment 8919381 [details]
Bug 1401207 - Implement ./mach talos --subtests to allow filtering DAMP tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190230/diff/2-3/

Comment 16

[Alexandre Poirot [:ochameau]]

Oops, I was convinced eslint was always run on try, but looks like it is not when pushing only for DAMP tests...

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3219e7712baa
Refactor DAMP to more easily filter subtests. r=bgrins

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7eb824835640
Refactor DAMP to more easily filter subtests. r=bgrins
https://hg.mozilla.org/integration/autoland/rev/475c4262f5e5
Implement ./mach talos --subtests to allow filtering DAMP tests. r=jmaher

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7eb824835640
https://hg.mozilla.org/mozilla-central/rev/475c4262f5e5