1362013 - Code coverage build should run SpiderMonkey tests

Status: NEW

(Testing :: Code Coverage, enhancement)

Description

[Marco Castelluccio [:marco]]

They are called SM-tc on TreeHerder, for example: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=33b92d9c40562dab3d7b602368c75619f1d793f7&filter-searchStr=linux%20x64%20debug%20sm

Comment 1

[Greg Mierzwinski [:sparky]]

We'll have to add linux64-ccov to this list at [1] to trigger spidermonkey tests. It's the only way I've found so far. 

[1]: https://dxr.mozilla.org/mozilla-central/source/taskcluster/taskgraph/try_option_syntax.py#142,344

Comment 2

[Greg Mierzwinski [:sparky]]

Here's a start to this work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c72169f097bd396cce513be48a34549792a88e6a

I've added linux64-ccov to the ridealong builds, but we need to find a way to specify we are in the ccov build so that we can transform the tasks attribute 'treeherder.platform' (or ['treeherder']['platform']) to 'linux64-ccov/opt'.

The job-defaults change works, but as you can see the fuzzing is left in linux64/opt because it's platform setting overrides the defaults.

Another option that may or may not be possible is to use a keyed-by attribute for 'treeherder.platform'.

Comment 3

[Steve Fink [:sfink] [:s:]]

That change isn't going to do anything. If I'm understanding correctly, for ccov you ordinarily hack the platform to be eg linux64-ccov. When you transform tests, if the platform contains 'ccov' then you pass in extra flags to mozharness and bump the instance-size to large.

The sm jobs do not use mozharness, and are build jobs not test jobs.

I'll upload a patch for what I think the taskcluster part would need to look like. Actually implementing ccov for the jobs is separate; I'll have to reverse-engineer what mozharness is doing with that flag and port it to autospider.py.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Make ccov versions of spidermonkey jobs

Dustin - what do you think of this approach? (No rush to respond; I can always flip this to a review request when/if I manage to make the actual job do something with the env var it's getting.)

The most questionable part can be seen in the kind.yml file:

    run:
        using: spidermonkey
        support-code-coverage:
            by-run.using:
                spidermonkey: true
                default: false

This may just be excess complexity. The simpler alternative would be to not have anything at all in kind.yml, but programmatically choose based on run.using=spidermonkey. (I never override this default in the current patch.) I did it this way because... uh... well, mostly because I started on this route and decided to finish it. But in the final version, I'll probably want to use this as the default but not do ccov variants of the weirder run.using=spidermonkey jobs (msan, tsan, etc.) And in fact, I may want to turn it on for some other run.using values like spidermonkey-package. I'm not entirely sure yet; I'll need to implement the darn thing first.

Or, I could just set the default to 'true' and turn it off later for the other run.using values.

I guess for now I'm mostly asking whether by-x.y.z offends your sensibilities. If not, I still may or may not make use of it in the end.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8936989 [details] [diff] [review]
Make ccov versions of spidermonkey jobs

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

Regarding by-run.using, this feels like a case that can be handled in the transform.  Something like


```python
yield job

# jobs with run.using == spidermonkey also support running
# with code coverage enabled so create a distinct job with
# the CODE_COVERAGE flag set
if job['run']['using'] == 'spidermonkey':
    ..
    yield job
```

::: taskcluster/ci/config.yml
@@ +66,5 @@
>          'linux':
>              - 'linux-l10n'
>              - 'sm-arm-sim-linux32'
> +        'linux-ccov':
> +            - 'sm-arm-sim-linux32-ccov'

This `ridealong-builds` is intended to "trick" people into running additional jobs; for example, if you type `-p android-api-16` (in the legacy try syntax only) you automatically get `android-api-16-l10n` even if you didn't want it.  Is that necessary here?  Will someone running `-p linux-ccov` know what to do with the results from `sm-arm-sim-linux32-ccov`?

::: taskcluster/taskgraph/transforms/job/spidermonkey.py
@@ +27,5 @@
>      Required('spidermonkey-variant'): basestring,
> +
> +    # Whether to gather code coverage data
> +    Optional('support-code-coverage'): bool,
> +    Optional('code-coverage'): bool,

For e10s, we allow True, False, and "both", and then split the "both" into a job with True and a job with False.  I wonder if something similar could be done here?

Comment 6

[Steve Fink [:sfink] [:s:]]

(In reply to Dustin J. Mitchell [:dustin] from comment #5)
> Comment on attachment 8936989 [details] [diff] [review]
> Make ccov versions of spidermonkey jobs
> 
> Review of attachment 8936989 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Regarding by-run.using, this feels like a case that can be handled in the
> transform.  Something like
> 
> 
> ```python
> yield job
> 
> # jobs with run.using == spidermonkey also support running
> # with code coverage enabled so create a distinct job with
> # the CODE_COVERAGE flag set
> if job['run']['using'] == 'spidermonkey':
>     ..
>     yield job
> ```

Right, that's what I said too. But I still don't know whether that's actually the signal I want to use to decide whether to split off a coverage job. I guess once I've figured out how to actually make these jobs and how they work, then if the jobs that want coverage are identical to those with run.using==spidermonkey, then I'll just do it in the transform. Otherwise, I'll decide based on how many exceptions there are. If exceptions are common, then I'll just mark each job explicitly. If exceptions are rare, then I'll consider using this as a default and overriding it in the few instances necessary.

...though having said that, now I'm leaning towards being explicit even if there are no exceptions. Oh well. I'm not going to do this work now; I'll decide later.

> ::: taskcluster/ci/config.yml
> @@ +66,5 @@
> >          'linux':
> >              - 'linux-l10n'
> >              - 'sm-arm-sim-linux32'
> > +        'linux-ccov':
> > +            - 'sm-arm-sim-linux32-ccov'
> 
> This `ridealong-builds` is intended to "trick" people into running
> additional jobs; for example, if you type `-p android-api-16` (in the legacy
> try syntax only) you automatically get `android-api-16-l10n` even if you
> didn't want it.  Is that necessary here?

I'm not sure. I'll ask the ccov people.

> Will someone running `-p
> linux-ccov` know what to do with the results from `sm-arm-sim-linux32-ccov`?

Yes, I'm pretty sure they will. I'm assuming they'll generate the same code coverage output files that all the other builds do, which I think already get manually aggregated into a global coverage view.

> ::: taskcluster/taskgraph/transforms/job/spidermonkey.py
> @@ +27,5 @@
> >      Required('spidermonkey-variant'): basestring,
> > +
> > +    # Whether to gather code coverage data
> > +    Optional('support-code-coverage'): bool,
> > +    Optional('code-coverage'): bool,
> 
> For e10s, we allow True, False, and "both", and then split the "both" into a
> job with True and a job with False.  I wonder if something similar could be
> done here?

Ooh, yeah, that approach feels better than two separate attributes. Thanks.

Comment 7

[Greg Mierzwinski [:sparky]]

:sfink, thanks for working on this! :)

I don't think we would always want to have sm builds running alongside the standard build on try (for example, when we are testing ccov-only failures, it would be a large waste of resources to have those additional builds running). That said, I'm pretty sure that we would want it running on mozilla-central alongside the standard builds by default, assuming it has code coverage that is unique to it. :marco, :ekyle, :jmaher would be able to help with this decision more than I can.

You're right, you will get the same data generated from it. But you'll have to speak with :ekyle, and :marco about whether or not it's data will be aggregated into active data or codecov.io for visualization since these are different builds.

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Greg Mierzwinski [:sparky] from comment #7)
> :sfink, thanks for working on this! :)
> 
> I don't think we would always want to have sm builds running alongside the
> standard build on try (for example, when we are testing ccov-only failures,
> it would be a large waste of resources to have those additional builds
> running). That said, I'm pretty sure that we would want it running on
> mozilla-central alongside the standard builds by default, assuming it has
> code coverage that is unique to it. :marco, :ekyle, :jmaher would be able to
> help with this decision more than I can.

We definitely want it to run for mozilla-central. I don't know about try. I see the normal linux64-debug builds do run the SM jobs too, so maybe we want the same? Joel what do you think?

> You're right, you will get the same data generated from it. But you'll have
> to speak with :ekyle, and :marco about whether or not it's data will be
> aggregated into active data or codecov.io for visualization since these are
> different builds.

Yes, we will injest it to have full coverage information.

Steve, the coverage builds are linux64-ccov and win64-ccov, so we should only define the SM builds/tests for those platforms.

Comment 9

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

We should support try, but not by default- ideally users will have to specifically pick linux64-ccov platform or find specific ccov jobs in ./mach try fuzzy.  If we can make it show up as the same platform as linux64-ccov, then it will be easier for us to visually understand and also to ingest into coverage reports.

Comment 10

[Steve Fink [:sfink] [:s:]]

Unassigning as I'm not working on this and won't get to it anytime soon.

Although if someone did the work of figuring out exactly what needs to be done during the build and tests (I guess that's whatever mozharness is doing?), then I could probably find time to finish this up.

Comment 11

[Marco Castelluccio [:marco]]

Steve, would you have time to work on this? Most of this should just be duplicating the current SM jobs, and I could help you with the coverage parts (just need a few build options and to run a script at the end).

Comment 12

[Steve Fink [:sfink] [:s:]]

(In reply to Marco Castelluccio [:marco] from comment #11)

Steve, would you have time to work on this? Most of this should just be duplicating the current SM jobs, and I could help you with the coverage parts (just need a few build options and to run a script at the end).

I won't have time immediately. I have a couple of things I need to work on right away, then I would have some bandwidth available.

But you don't need to manually duplicate the SM job configuration, which feels unnecessarily error-prone to me. The patch attached to this bug (probably somewhat bitrotted at this point) duplicates the job names, but automatically creates the task configurations (based on a YAML field.) Dustin had some comments and change requests, but if someone wanted to take it over then that part could probably be beaten into shape relatively easily.

Can you comment here with the build options? I could at least add those to the spidermonkey autospider.py run script, using either platform names (as with the patch currently attached here) or an environment variable. Or even a command-line option, though that would require some more taskgraph plumbing work too.

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Steve Fink [:sfink] [:s:] from comment #12)

Can you comment here with the build options? I could at least add those to the spidermonkey autospider.py run script, using either platform names (as with the patch currently attached here) or an environment variable. Or even a command-line option, though that would require some more taskgraph plumbing work too.

The first step would be to enable C/C++ coverage, which only requires adding "--coverage" to CFLAGS, CXXFLAGS and LDFLAGS. You'll probably also want to define MOZ_CODE_COVERAGE (as done at https://searchfox.org/mozilla-central/source/build/moz.configure/toolchain.configure#1785).
Here are the options we use in the linux64 build (https://searchfox.org/mozilla-central/source/browser/config/mozconfigs/linux64/code-coverage), but I assume most of them are unrelated to SM builds.

Then, we can enable Rust coverage later on.

At the end of the build and the tests execution, we'll need to parse the results doing something like https://searchfox.org/mozilla-central/source/testing/parse_build_tests_ccov.py.