Closed
Bug 1355180
Opened 8 years ago
Closed 8 years ago
Make the default for run-on-projects for tests default to that for the parent build
Categories
(Firefox Build System :: Task Configuration, task)
Firefox Build System
Task Configuration
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla55
People
(Reporter: dustin, Assigned: dustin)
References
Details
Attachments
(2 files, 3 obsolete files)
So if you have
someplatform:
run-on-projects: ['oak']
and a test platform
platsomeform: # mozilla doesn't spell things the same way twice, after all
build-platform: someplatform
test-sets:
- common-tests
then someplatform will be built on all platforms, because most of the common-tests default to `run-on-projects: [all]`. Thus even though the build isn't selected as a target, the tests are, and they require the build.
If, instead, those tests had something like `run-on-projects: build-platform-projects`, that could be translated (in this case) to [], and the tests wouldn't be targetted, so the build wouldn't either.
:ting, :jmaher, thoughts?
Flags: needinfo?(jmaher)
Flags: needinfo?(janus926)
Comment 1•8 years ago
|
||
I like this idea in general. There are a lot of details and exceptions, but I think this will reduce a lot of confusion going forward.
Flags: needinfo?(jmaher)
Comment 2•8 years ago
|
||
Why don't we just skip untargetted build even if the tests are selected by [all]?
Flags: needinfo?(janus926)
Updated•8 years ago
|
Assignee | ||
Comment 3•8 years ago
|
||
Good question. Thinking about that question a little, I see we could actually get some nice error messages for users:
build test result
----- ---- ------
[oak] [oak] build and test only on oak
[all] [oak] build everywhere and test only on oak
[all] [all] build and test everywhere
[oak] built-projects build and test only on oak
[oak] [all] error: tests without a build
For a test (in tests.yml), the default would be "built-projects". However, it would help avoid this situation:
someplatform:
run-on-projects: ['oak']
and a test
sometest:
run-on-projects: ['oak', 'jamun']
which might be the first idea of someone interested in running `sometest` on jamun. Currently, this will cause someplatform to "silently" build on jamun, even though only oak is listed. With this change, the `./mach taskgraph` invocation would fail with an error saying that test-someplatform/sometest has tests but no build on the jamun project.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8859254 [details]
Bug 1355180: introduce run-on-projects: built-projects, and use it;
https://reviewboard.mozilla.org/r/131270/#review133842
this looks good- please ensure docs for taskcluster are up to date :)
Attachment #8859254 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Was there a particular piece of docs you're thinking of? The change is documented in the schema (tests.py).
Comment 8•8 years ago
|
||
I am thinking of here:
https://dxr.mozilla.org/mozilla-central/source/taskcluster/docs/attributes.rst#22
and I believe there is a fancier site somewhere on the internet with similar information.
Assignee | ||
Comment 9•8 years ago
|
||
Oops, I forgot there was a stylo-specific followon I wanted to push.
I noticed that the stylo *builds* omit try, but the stylo-specific tests do not. The result is that a push to try with -p all would include the tests, which require the builds -- so the builds would be run anyway. Things get a little simpler in the test configuration if we just admit this and add "try" to the run-on-projects for the builds.
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Joel Maher ( :jmaher) from comment #8)
> I am thinking of here:
> https://dxr.mozilla.org/mozilla-central/source/taskcluster/docs/attributes.
> rst#22
>
> and I believe there is a fancier site somewhere on the internet with similar
> information.
The 'built-projects' never appears as an attribute, so that would be a confusing place to write it down. It just indicates that the test should copy its run_on_projects attribute from the build it depends on. I could write it in `kinds.rst`, but it seems *way* more specific than the other content of that file.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8859253 -
Attachment is obsolete: true
Updated•8 years ago
|
Attachment #8859273 -
Flags: review?(kmoir) → review+
Assignee | ||
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8859273 [details]
Bug 1355180: remove special-casing of stylo tests on try;
https://reviewboard.mozilla.org/r/131286/#review133970
Kim, for some reason the r+ didn't register in mozreview. While I have your attention, does this match your understanding of stylo -- that the stylo builds should occur on try with `-p all`? I've made a try job linked to this review request with just such an option, if you want to take a look.
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8859273 [details]
Bug 1355180: remove special-casing of stylo tests on try;
https://reviewboard.mozilla.org/r/131286/#review135376
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 17•8 years ago
|
||
I rebased and re-tested. Still no diff.
Comment 18•8 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s 51511e61d599 -d 5d07afcca48a: rebasing 392257:51511e61d599 "Bug 1355180: introduce run-on-projects: built-projects, and use it; r=jmaher"
merging taskcluster/ci/test/tests.yml
warning: conflicts while merging taskcluster/ci/test/tests.yml! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8859273 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8862635 -
Attachment is obsolete: true
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8862642 [details]
Bug 1355180: remove special-casing of stylo tests on try;
https://reviewboard.mozilla.org/r/134492/#review137474
carry forward kim's r+
Attachment #8862642 -
Flags: review+
Comment 25•8 years ago
|
||
Pushed by dmitchell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ff20d246999
introduce run-on-projects: built-projects, and use it; r=jmaher
https://hg.mozilla.org/integration/autoland/rev/3448d57a9be8
remove special-casing of stylo tests on try; r=dustin
Assignee | ||
Updated•8 years ago
|
Attachment #8862642 -
Flags: review?(kmoir)
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3ff20d246999
https://hg.mozilla.org/mozilla-central/rev/3448d57a9be8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Product: TaskCluster → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•