Error out when requesting compiled code tests + artifact builds on Try

RESOLVED FIXED in Firefox 52

Status

defect
P1
normal
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: jgriffin, Assigned: maja_zf)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [artifact-builds-on-automation])

Attachments

(2 attachments)

Reporter

Description

3 years ago
Once we have the ability to trigger artifact builds instead of regular ones on Try (see bug 1278698), we'll need the ability to prevent certain suites from being triggered against these, as they will fail: gtest, cpptests, and possible a couple of others.

Probably the easiest way to do this is to change the hg try hook so that the try push aborts if you specify both the flag for producing artifact builds and one of the unsupported test suites.

Comment 1

3 years ago
Instead of using the hg try hook we might want to use |mach try| to allow this sort of development since we have more control over it and IIRC we want to keep the hg hook as light as possible.

Asking gps will be the right call between choose the two approaches.
Whiteboard: [skip-builds] → [artifact-builds-on-automation]

Updated

3 years ago
Blocks: artifact-builds-on-try
No longer blocks: 1278698
Priority: -- → P1
We have an issue whenever someone tries to pass `-u all` (which I expect is very common). If this makes it into try syntax, the buildbot try parser will schedule jobs like gtests and cpp tests, and I don't know of a knob we can tweak to make that not happen.

If people use mach try, we can detect `-u all` + artifact builds, and replace `-u all` in the generated syntax with the set difference between all the suites and those that aren't interesting to artifact builds.

Longer term, I think we'll want to have the artifact builds as taskcluster tasks that don't have compiled tests that depend on them.

As a shorter term solution, perhaps we could have test jobs determine they're triggered from an artifact build, and mark themselves as skipped/not-run instead of running and failing.

Updated

3 years ago
Assignee: nobody → mjzffr
Comment hidden (mozreview-request)
Comment on attachment 8787028 [details]
Bug 1278702 - Error out when compiled-code test is requested with artifact build;

I took a first look at the |mach try| route today. Sharing initial idea to continue discussion. I'm thinking we can maintain a list of compiled-code tests  (which are those, by the way?) in AutoTry and check against that and |-u all| whenever we have the --artifact flag. If I'm totally off or missing something, let me know. Thanks!
Attachment #8787028 - Flags: feedback?(cmanchester)
Comment on attachment 8787028 [details]
Bug 1278702 - Error out when compiled-code test is requested with artifact build;

This is good, but we need to add at least 'gtest' to this list, I think jit tests too, and handle '-u all' in some fashion (it's probably a common choice).
Attachment #8787028 - Flags: feedback?(cmanchester)
I think the more complete solution here will be the last thing I mentioned in comment 2, which would involve modifying mozharness to detect when it's being asked to run a compiled code test against an artifact build and bail out early with a status somewhere between red and green so people don't interpret the failing jobs as caused by their push.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8787028 [details]
Bug 1278702 - Error out when compiled-code test is requested with artifact build;

https://reviewboard.mozilla.org/r/75866/#review77902

::: testing/tools/autotry/autotry.py:388
(Diff revision 3)
>          if not suites:
>              raise ValueError("No tests found matching filters")
>  
> +        if extras.get('artifact'):
> +            rejected = []
> +            for suite in suites.keys():

Regarding '-u all', the options I see are:

1. As suggested, compute a set difference to get tests that are compatible with --artifact. This requires adding and maintaining a long list of all possible test suites in `autotry.py`.

2. Let -u all through in mach try and let mozharness end the incompatible jobs early.

3. Reject -u all in mach try when --artifact is present, which I assume will make some users unhappy.

What do you think? I favour Option 2, but maybe there's a better approach to Option 1?
Comment on attachment 8792175 [details]
Bug 1278702 - Fail early in compiled-code tests with --artifact Try syntax;

https://reviewboard.mozilla.org/r/79378/#review77904

::: testing/mozharness/scripts/desktop_unittest.py:29
(Diff revision 1)
>  from mozharness.base.errors import BaseErrorList
>  from mozharness.base.log import INFO, ERROR
>  from mozharness.base.script import PreScriptAction
>  from mozharness.base.vcs.vcsbase import MercurialScript
>  from mozharness.mozilla.blob_upload import BlobUploadMixin, blobupload_config_options
> +from mozharness.mozilla.buildbot import TBPL_EXCEPTION

Perhaps this isn't necessary. I added it while looking at how the job might report something other than testfailed/busted to Treeherder. I get the impression that I can't influence this from within mozharness. I'll ask around on Monday.

So, right now the incompatible jobs are reported as testfailed on Treeherder. https://hg.mozilla.org/try/rev/4fd765b672a2c8a837f3596e638480f7a60ce004 -- see Failure Summary

Comment 12

3 years ago
#1 --> Do we believe the burden of maintaining the list might be too much?

#2 --> I'm not thrilled with the idea of having a bunch of failed jobs on a push which could hide potential issues on the valid jobs.

#3 --> give them the exact command they should run OR show them which suites we're dropping and ask them if they're OK to proceed with reduced subset.

Comment 13

3 years ago
mozreview-review
Comment on attachment 8792175 [details]
Bug 1278702 - Fail early in compiled-code tests with --artifact Try syntax;

https://reviewboard.mozilla.org/r/79378/#review77922
Attachment #8792175 - Flags: review?(armenzg) → review+

Comment 14

3 years ago
You should probably file a bug for TaskCluster to deal properly with Mozharness exit codes.
Probably talk with garndt.

Your script tries to give an exit code of 3 while TC exits with 255.
> [task 2016-09-17T03:20:42.693361Z] 03:20:42  WARNING - setting return code to 3
...
> [taskcluster 2016-09-17 03:20:49.396Z] Unsuccessful task run with exit code: 255 completed in 29.299 seconds
(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #12)
> #1 --> Do we believe the burden of maintaining the list might be too much?
> 

To get truly all jobs, we'd have a long list that could easily fall out-of-date, which is why I hesitate. It seems harmful to have the user believe that all tests (except compiled-code) are running, when we can't actually guarantee that.

Ultimately, I think we want these checks to be in TaskCluster's try syntax parser. I think it computes all possible jobs based on the task graph. (It would even be easy to implement a --not flag if we wanted to.) 

> #2 --> I'm not thrilled with the idea of having a bunch of failed jobs on a
> push which could hide potential issues on the valid jobs.
> 

I agree. Some status other than 'testfailed' or 'busted' should be used. 

Adjusting |mach try| reduces the frequency of these invalid jobs, but won't avoid them altogether since there are lots of try pushes that come from elsewhere.


> #3 --> give them the exact command they should run OR show them which suites
> we're dropping and ask them if they're OK to proceed with reduced subset.

Yes, I guess this is a compromise with Option 1 in that it asks the user to make a conscious choice. 

"You asked for |-u all| with |--artifact| but compiled-code tests (jittest, gtest, cppunit) can't run against an artifact build. Try this syntax instead; it lists major test suites: 
try -b do -p linux64 -u reftest,crashtest,xpcshell,mochitests,web-platform-tests,jsreftest,marionette,marionette-e10s,firefox-ui-functional,media-tests --artifact"

I prefer the idea of maintaining a short list of major suites without claiming that these are all possible tests.
(In reply to Armen Zambrano [:armenzg] (EDT/UTC-4) from comment #14)
> You should probably file a bug for TaskCluster to deal properly with
> Mozharness exit codes.
> Probably talk with garndt.
> 
> Your script tries to give an exit code of 3 while TC exits with 255.
> > [task 2016-09-17T03:20:42.693361Z] 03:20:42  WARNING - setting return code to 3
> ...
> > [taskcluster 2016-09-17 03:20:49.396Z] Unsuccessful task run with exit code: 255 completed in 29.299 seconds

I've fixed this by passing self.return_code to self.fatal() -- self.fatal does a SystemExit with -1 by default, so it wasn't TaskCluster's fault. Now I have:

> [taskcluster 2016-09-19 14:41:54.764Z] Unsuccessful task run with exit code: 3 completed in 44.829 seconds

This looks purple on Treeherder when run with Buildbot. Unfortunately, TC still reports this to Treeherder as testfailed, see Bug 1186848.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96292b40d41a62e8e96ff7a8706f7f42b74604e6&selectedJob=27651991
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

3 years ago
mozreview-review
Comment on attachment 8787028 [details]
Bug 1278702 - Error out when compiled-code test is requested with artifact build;

https://reviewboard.mozilla.org/r/75866/#review78614

This looks good, thanks!

::: testing/tools/autotry/autotry.py:180
(Diff revision 4)
> +        "cppunit",
> +        "gtest",
> +        "jittest",
> +    ]
> +
> +    major_suites = [

Maybe call this "known" or "common" suites.

::: testing/tools/autotry/autotry.py:442
(Diff revision 4)
> +            message = ('You asked for |-u all| with |--artifact| but compiled-code tests ({tests})'
> +                       ' can\'t run against an artifact build. Try this syntax instead;'
> +                       ' it lists major test suites:\n{try_syntax}')

I think this is a good solution for now. We can tweak it based on feedback from users (I really don't know if people would rather just have us push rather than stopping them to inform gtests etc. wont make it into their push).

I would re-word this a bit, to suggest that they select the tests they want by passing a more meaningful argument to "-u", and then offer this syntax as an example of something that will cover most things.
Attachment #8787028 - Flags: review?(cmanchester) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

3 years ago
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e4086709000d
Error out when compiled-code test is requested with artifact build; r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/e9aa1c694bb1
Fail early in compiled-code tests with --artifact Try syntax; r=armenzg

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e4086709000d
https://hg.mozilla.org/mozilla-central/rev/e9aa1c694bb1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
See Also: → 1459728
You need to log in before you can comment on or make changes to this bug.