Open Bug 1362013 Opened 7 years ago Updated 2 years ago

Code coverage build should run SpiderMonkey tests

Categories

(Testing :: Code Coverage, enhancement)

Product:
Testing
Component:
Code Coverage
Version:
Version 3
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: marco, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Make ccov versions of spidermonkey jobs
(deleted), patch
dustin
: feedback+
Details | Diff | Splinter Review
Blocks: 1368115
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
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'.
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.
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.
Attachment #8936989 - Flags: feedback?(dustin)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
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?
Attachment #8936989 - Flags: feedback?(dustin) → feedback+
(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.
: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.
(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.
Flags: needinfo?(jmaher)
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.
Flags: needinfo?(jmaher)
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.
Assignee: sphink → nobody
Status: ASSIGNED → NEW

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).

Flags: needinfo?(sphink)

(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.

(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.

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: