Closed
Bug 1188330
Opened 9 years ago
Closed 9 years ago
Use intree mozharness for taskcluster tests
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Release Engineering
Applications: MozharnessCore
Tracking
(firefox43 fixed, b2g-master affected)
RESOLVED
FIXED
People
(Reporter: gerard-majax, Assigned: edgar)
References
Details
Attachments
(1 file, 2 obsolete files)
We need to change the name of the binary for at least Gu: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66af40f9591
That would be much easier to work on with intree mozharness.
Flags: needinfo?(jlund)
Comment 1•9 years ago
|
||
We need to switch tester docker image to in tree mozharness [0]. As tester docker doesn't need the full gecko repo, we can use archiver_client [1] for that.
[0] https://dxr.mozilla.org/mozilla-central/source/testing/docker/tester/bin/entrypoint
[1] http://jordan-lund.ghost.io/mozharness-goes-live-in-the-tree/
Comment 2•9 years ago
|
||
We should also clean up mozilla-taskcluster not to populate the MOZHARNESS_* environment variables in the generated task definition...
Comment 3•9 years ago
|
||
(In reply to Pete Moore [:pmoore][:pete] from comment #2)
> We should also clean up mozilla-taskcluster not to populate the MOZHARNESS_*
> environment variables in the generated task definition...
Ah - this is done in the decision task - my bad. So we should update in-tree taskcluster-graph, i.e.
https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/mach_commands.py#l238
Comment 4•9 years ago
|
||
So to summarize:
1) mach taskcluster-graph generates tasks containing mozharness environment variables
2) https://hg.mozilla.org/try/file/3d0f918b7e4a/testing/docker/tester/bin/entrypoint#l23 checks out mozharness using those values
Instead, the in-tree task definitions should use the in-tree mozharness (under /testing/mozharness) and the mozharness environment variables should be removed, and the entrypoint should be changed not to check out mozharness.
=========
To see this in the code:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66af40f9591
=> https://tools.taskcluster.net/task-inspector/#OUlQhkr_ShWevhSoUL7gig/0
=> https://queue.taskcluster.net/v1/task/OUlQhkr_ShWevhSoUL7gig
=> https://tools.taskcluster.net/task-graph-inspector/#dSO_y8_jRtqmx328IeJrcg/
=> https://queue.taskcluster.net/v1/task/M_D8ClrvSxS3ux2EZnhHYg
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/mach_commands.py#l207
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/mach_commands.py#l265
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/taskcluster_graph/commit_parser.py#l202
=> here it gets a little hazy...
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/tasks/tests/b2g_gaia_unit.yml#l3
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/tasks/test.yml#l39
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/mach_commands.py#l281
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/mach_commands.py#l266
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/mach_commands.py#l44
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/taskcluster/mach_commands.py#l24
=> https://hg.mozilla.org/mozilla-central/file/2ee9895e032c/testing/mozharness/mozharness.json#l2
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Comment 6•9 years ago
|
||
hrm, I don't see this bug as the same as bug 1185643. Actually I see bug 1185643 more as a meta bug, because there so many pieces to be patched.
See: https://etherpad.mozilla.org/gecko-mozharness-migration
Comment 7•9 years ago
|
||
does https://bugzilla.mozilla.org/show_bug.cgi?id=1188330#c4 answer the needinfo? Is there a way I can help here? Thanks wcosta and pmoore for the work thus far.
Updated•9 years ago
|
Flags: needinfo?(jlund)
Comment 8•9 years ago
|
||
In bug 1185643 I added a comment to `testing/taskcluster/mach_commands.py` indicating that `load_mozharness_info` and friends should be removed once nothing uses the mozharnses variables. I *suspect* this is the bug that will finally allow that to be removed.
Comment 9•9 years ago
|
||
reopening in agreement with comment 6 and since this isn't done yet :)
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Updated•9 years ago
|
Blocks: b2g-emulator-x86-KK
Updated•9 years ago
|
blocking-b2g: --- → 2.5+
Updated•9 years ago
|
status-b2g-master:
--- → affected
Comment 10•9 years ago
|
||
Some words about why this is a 2.5+.
Now emulator tests on TC are failed due to the error |The in-tree configuration file '/home/worker/build/tests/config/mozharness/b2g_emulator_config.py' does not exist!It must be added to 'gecko/testing/config/mozharness/b2g_emulator_config.py'. See bug 1035551 for more details.|
According to bug 1181261 comment 26, it points this bug is the key to those test failures. That's why we marked this 2.5+ for B2G.
[1] https://treeherder.allizom.org/#/jobs?repo=try&revision=6c932d1134af
Comment 11•9 years ago
|
||
double checking 1181261 isn't involved via https://bugzilla.mozilla.org/show_bug.cgi?id=1181261#c29
Comment 12•9 years ago
|
||
Hi Jonathan,
Can you find someone who can help here? This bug is the major blocker for emulator on TC.
Thanks.
Flags: needinfo?(jgriffin)
Comment 13•9 years ago
|
||
(In reply to Hsin-Yi Tsai (OOO Sep. 4 ~ Sep. 20) [:hsinyi] from comment #10)
> Some words about why this is a 2.5+.
>
> Now emulator tests on TC are failed due to the error |The in-tree
> configuration file
> '/home/worker/build/tests/config/mozharness/b2g_emulator_config.py' does not
> exist!It must be added to
> 'gecko/testing/config/mozharness/b2g_emulator_config.py'. See bug 1035551
> for more details.|
>
> According to bug 1181261 comment 26, it points this bug is the key to those
> test failures. That's why we marked this 2.5+ for B2G.
>
> [1] https://treeherder.allizom.org/#/jobs?repo=try&revision=6c932d1134af
Sorry, no one ever pasted a bug or failure into bug 1181261, so I didn't realize that emulator-kk tests were busted! While it's true that fixing this bug would also fix your problem, I can just temporarily add the config file back to unblock you guys in the short term.
Thanks for double checking Jordan.
Flags: needinfo?(jgriffin)
Comment 14•9 years ago
|
||
Fix just landed in the other bug. Sorry for the confusion, I misinterpreted the problem from https://bugzilla.mozilla.org/show_bug.cgi?id=1181261#c27 . For future reference, if you have an error, please paste it in the bug :)
blocking-b2g: 2.5+ → ---
Comment 15•9 years ago
|
||
(In reply to Andrew Halberstadt [:ahal] from comment #14)
> Fix just landed in the other bug. Sorry for the confusion, I misinterpreted
> the problem from https://bugzilla.mozilla.org/show_bug.cgi?id=1181261#c27 .
> For future reference, if you have an error, please paste it in the bug :)
Thank you Andrew!
B.t.w. do you happen to know which channel we could use to communicate with the release team people that FxOS team is currently working on emu-kk on Treeherder *Staging* and it would be much appreciation that teams could pay attention to that?
Comment 16•9 years ago
|
||
The only way to make sure your work isn't broken is to get sheriffs to look at it and back things out that break them. Unfortunately they won't look at things that are already perma-orange. I know it's painful, I've gone through the process of trying to get tests stood up on a new platform many times myself.
But let's take this conversation to e-mail, it isn't related to this bug.
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Wander Lairson Costa [:wcosta] from comment #1)
> We need to switch tester docker image to in tree mozharness [0]. As tester
> docker doesn't need the full gecko repo, we can use archiver_client [1] for
> that.
>
> [0]
> https://dxr.mozilla.org/mozilla-central/source/testing/docker/tester/bin/
> entrypoint
>
> [1] http://jordan-lund.ghost.io/mozharness-goes-live-in-the-tree/
I have a different idea here. What about,
1. Packaging the mozharness, let's say mozharness.zip, and put it under public/build for each build. (something like what we did for target.*.tests.zip)
2. Then the tester docker can download a mozharness.zip from a specific build.
The advantage is of this approach is:
1. The revision of mozharness will match with the build/test we triggered.
2. Any changes in mozharness can also be secured.
Hi Wander, may I have some feedback from you? I can provide a WIP patch and try result if you are interested.
Thank you.
Flags: needinfo?(wcosta)
Comment 18•9 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #17)
> (In reply to Wander Lairson Costa [:wcosta] from comment #1)
> > We need to switch tester docker image to in tree mozharness [0]. As tester
> > docker doesn't need the full gecko repo, we can use archiver_client [1] for
> > that.
> >
> > [0]
> > https://dxr.mozilla.org/mozilla-central/source/testing/docker/tester/bin/
> > entrypoint
> >
> > [1] http://jordan-lund.ghost.io/mozharness-goes-live-in-the-tree/
>
> I have a different idea here. What about,
> 1. Packaging the mozharness, let's say mozharness.zip, and put it under
> public/build for each build. (something like what we did for
> target.*.tests.zip)
> 2. Then the tester docker can download a mozharness.zip from a specific
> build.
>
> The advantage is of this approach is:
> 1. The revision of mozharness will match with the build/test we triggered.
> 2. Any changes in mozharness can also be secured.
>
> Hi Wander, may I have some feedback from you? I can provide a WIP patch and
> try result if you are interested.
>
I don't have a strong opinion on this, but if archive_client supports checking out with a specific rev, we can achieve the same effect just by putting the revision in the test task.
Flags: needinfo?(wcosta)
Comment 19•9 years ago
|
||
I prefer Edgar's solution, if possible - especially if that .zip only contains the MH files, and not the hg revision history. It mirrors the test zip idea nicely, and it looks like it's only about 3MB.
Archiver is basically an interim solution until either hg.m.o supports caching tarballs (archiver is basically a purpose-specific cache in front of hg.m.o, after all) or we extend proxxxy (the generic releng proxy) to handle hg.m.o tarballs or hg.m.o supports subdirectory checkouts (which mercurial itself doesn't support yet, but is working on). As such, I'd rather avoid introducing dependencies on archiver where possible, as we'll just need to unwind those dependencies later.
Comment 20•9 years ago
|
||
> The advantage is of this approach is:
> 1. The revision of mozharness will match with the build/test we triggered.
archiver archives mh from a specific repo+changeset once and uploads it to s3. both builds and tests in buildbot use this archive. iow - you can download by rev
though I hear dustin in not wanting to add more deps to this if an alternative is really easy
Assignee | ||
Comment 21•9 years ago
|
||
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8657561 [details] [diff] [review]
WIP, patch, v1
Review of attachment 8657561 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for all your comments.
Here is the WIP patch:
1. Package the mozharness when generating the test zip.
2. Move mozharness.zip to $HOME/artifacts. [1]
3. Generate mozharness url of a specific build and pass it to test task.
4. Test task downloads the mozharness.zip and unzip it. [2]
Here is the try runs for your reference: https://treeherder.allizom.org/#/jobs?repo=try&revision=408bc7975357&group_state=expanded
(I use my own docker hub repository for testing)
I am not sure who should I send feedback? to, so I just send to you all. Hope this won't bother you too much.
Thank you.
[1] You can see mozharness.zip in task inspector: https://tools.taskcluster.net/task-inspector/#_SkSA7fiRQeFqmkL4rkG1w/0
[2] Logs of CPP test for your reference: https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/4o2JhH34Tyu_AzwDRYOH2A/0/public/logs/live_backing.log
Attachment #8657561 -
Flags: feedback?(wcosta)
Attachment #8657561 -
Flags: feedback?(jlund)
Attachment #8657561 -
Flags: feedback?(dustin)
Comment 23•9 years ago
|
||
Comment on attachment 8657561 [details] [diff] [review]
WIP, patch, v1
Adding :garndt for feedback.
Attachment #8657561 -
Flags: feedback?(garndt)
Comment 24•9 years ago
|
||
Most of the tests failed, but I don't know if it is related to the patch.
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Wander Lairson Costa [:wcosta] from comment #24)
> Most of the tests failed, but I don't know if it is related to the patch.
No, it isn't related to this patch, most of the test on taskcluster are still failed even without this patch.
Here is the current situation:
We are trying to stabilize the test in bug 1144970 (mainly focus on emulator-x86-kk). But I realise making some tests green might need to change mozharness as well, like bug 1200928 (I guess there might be more). So I would like to move taskcluster tests to use intree mozharness first. Then we could focus on intree one if any changes needed in mozharness.
Thank you.
Comment 26•9 years ago
|
||
Comment on attachment 8657561 [details] [diff] [review]
WIP, patch, v1
Review of attachment 8657561 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm, just a remainder we need to check if it is necessary to modify phone tasks too.
::: testing/docker/tester/VERSION
@@ +1,1 @@
> +0.3.7.3
I think the convention is 3 version numbers.
Attachment #8657561 -
Flags: feedback?(wcosta) → feedback+
Comment 27•9 years ago
|
||
Comment on attachment 8657561 [details] [diff] [review]
WIP, patch, v1
Review of attachment 8657561 [details] [diff] [review]:
-----------------------------------------------------------------
looks good but I am not familiar with TC enough to review this :)
Attachment #8657561 -
Flags: feedback?(jlund)
Comment 28•9 years ago
|
||
Comment on attachment 8657561 [details] [diff] [review]
WIP, patch, v1
Review of attachment 8657561 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/docker/tester/REGISTRY
@@ +1,1 @@
> +edgarchen
Don't forget to push to taskcluster before landing, and change this (I always forget!)
::: testing/docker/tester/bin/entrypoint
@@ +1,3 @@
> #! /bin/bash -ex
>
> +test $MOZHARNESS_URL # mozharness url
If this variable isn't set, the only feedback you get in the logs is
```
test
```
Since you're running with -x, it can be helpful to log the things you're testing, similar to how build-linux.sh does.
https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/scripts/builder/build-linux.sh?offset=0#12
: MOZHARNESS_SCRIPT ${MOZHARNESS_SCRIPT}
@@ +19,5 @@
> # $ docker -v your_mozharness:/home/worker/mozharness ...
> #
> if [ ! -d mozharness ]; then
> + buildbot_step 'Download mozharness' wget -c $MOZHARNESS_URL
> + buildbot_step 'Unzip mozharness' unzip -q mozharness.zip
Won't this leave the mozharness directory in place in the workspace? In which case, [ ! -d mozharness ] will be false on the next run, and no new unzip will take place?
Attachment #8657561 -
Flags: feedback?(dustin) → feedback+
Comment 29•9 years ago
|
||
Comment on attachment 8657561 [details] [diff] [review]
WIP, patch, v1
Review of attachment 8657561 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/docker/tester/bin/entrypoint
@@ +1,3 @@
> #! /bin/bash -ex
>
> +test $MOZHARNESS_URL # mozharness url
From what I see this will only defined for builds that produce the mozharness.zip, which is just he emulator builds altered in this patch. What about b2g builds/tests that do not have this specified?
@@ +18,5 @@
> #
> # $ docker -v your_mozharness:/home/worker/mozharness ...
> #
> if [ ! -d mozharness ]; then
> + buildbot_step 'Download mozharness' wget -c $MOZHARNESS_URL
Is there any value in having this split across two buildbot steps?
Also, if this directory does not exist, it will try to pull down the mozharness zip for everything that inherits from test.yml.....what if the build (such as b2g desktop) doesn't have this?
@@ +19,5 @@
> # $ docker -v your_mozharness:/home/worker/mozharness ...
> #
> if [ ! -d mozharness ]; then
> + buildbot_step 'Download mozharness' wget -c $MOZHARNESS_URL
> + buildbot_step 'Unzip mozharness' unzip -q mozharness.zip
If this is loaded in the workspace, we probably want to get rid of the mozharness.zip as well so we're not saving that in the cache each time.
Attachment #8657561 -
Flags: feedback?(garndt) → feedback+
Assignee | ||
Comment 30•9 years ago
|
||
(In reply to Dustin J. Mitchell [:dustin] from comment #28)
> Comment on attachment 8657561 [details] [diff] [review]
> WIP, patch, v1
>
> Review of attachment 8657561 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: testing/docker/tester/REGISTRY
> @@ +1,1 @@
> > +edgarchen
>
> Don't forget to push to taskcluster before landing, and change this (I
> always forget!)
Sure, I will use taskcluster in formal patch.
But I don't have permission to push image to taskcluster, may need some help from you guys when landing the patch. :)
>
> ::: testing/docker/tester/bin/entrypoint
> @@ +1,3 @@
> > #! /bin/bash -ex
> >
> > +test $MOZHARNESS_URL # mozharness url
>
> If this variable isn't set, the only feedback you get in the logs is
>
> ```
> test
> ```
>
> Since you're running with -x, it can be helpful to log the things you're
> testing, similar to how build-linux.sh does.
> https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/scripts/
> builder/build-linux.sh?offset=0#12
> : MOZHARNESS_SCRIPT ${MOZHARNESS_SCRIPT}
Will do in formal patch. Thank you.
Assignee | ||
Comment 31•9 years ago
|
||
(In reply to Greg Arndt [:garndt] from comment #29)
> Comment on attachment 8657561 [details] [diff] [review]
> WIP, patch, v1
>
> Review of attachment 8657561 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: testing/docker/tester/bin/entrypoint
> @@ +1,3 @@
> > #! /bin/bash -ex
> >
> > +test $MOZHARNESS_URL # mozharness url
>
> From what I see this will only defined for builds that produce the
> mozharness.zip, which is just he emulator builds altered in this patch.
> What about b2g builds/tests that do not have this specified?
I will review all the build/test and do proper modification (and also remove the old mozharnees variables) in formal patch.
Thank you.
>
> @@ +18,5 @@
> > #
> > # $ docker -v your_mozharness:/home/worker/mozharness ...
> > #
> > if [ ! -d mozharness ]; then
> > + buildbot_step 'Download mozharness' wget -c $MOZHARNESS_URL
>
> Is there any value in having this split across two buildbot steps?
>
> Also, if this directory does not exist, it will try to pull down the
> mozharness zip for everything that inherits from test.yml.....what if the
> build (such as b2g desktop) doesn't have this?
Ditto
Assignee | ||
Updated•9 years ago
|
Summary: Use intree mozharness for Mulet Gb, Gbu, Gu and Li → Use intree mozharness for taskcluster tests
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 32•9 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5357a9da67e
Most test looks good, except all Gaia test on Mulet/B2G_Desktop busted.
Investigating ....
> Traceback (most recent call last):
> File "./mozharness/scripts/gaia_build_integration.py", line 14, in <module>
> from mozharness.mozilla.testing.gaia_test import GaiaTest
> File "/home/worker/mozharness/mozharness/mozilla/testing/gaia_test.py", line 28, in <module>
> class GaiaTest(TestingMixin, MercurialScript, TransferMixin, GaiaMixin, BlobUploadMixin):
> TypeError: Error when calling the metaclass bases
> Cannot create a consistent method resolution
> order (MRO) for bases BlobUploadMixin, VirtualenvMixin, object, GaiaMixin, TransferMixin
Attachment #8657561 -
Attachment is obsolete: true
Assignee | ||
Comment 33•9 years ago
|
||
Comment on attachment 8659786 [details] [diff] [review]
WIP, Patch, v2
Another error in Mulet reftests:
> 02:50:05 FATAL - Uncaught exception: Traceback (most recent call last):
> 02:50:05 FATAL - File "/home/worker/mozharness/mozharness/base/script.py", line 1693, in run
> 02:50:05 FATAL - self.run_action(action)
> 02:50:05 FATAL - File "/home/worker/mozharness/mozharness/base/script.py", line 1635, in run_action
> 02:50:05 FATAL - self._possibly_run_method(method_name, error_if_missing=True)
> 02:50:05 FATAL - File "/home/worker/mozharness/mozharness/base/script.py", line 1576, in _possibly_run_method
> 02:50:05 FATAL - return getattr(self, method_name)()
> 02:50:05 FATAL - File "./mozharness/scripts/mulet_unittest.py", line 73, in run_tests
> 02:50:05 FATAL - super(MuletUnittest, self).run_tests()
> 02:50:05 FATAL - File "/home/worker/mozharness/scripts/b2g_desktop_unittest.py", line 197, in run_tests
> 02:50:05 FATAL - cmd = self._query_abs_base_cmd(suite)
> 02:50:05 FATAL - File "/home/worker/mozharness/scripts/b2g_desktop_unittest.py", line 156, in _query_abs_base_cmd
> 02:50:05 FATAL - if suite not in self.config["suite_definitions"]:
> 02:50:05 FATAL - KeyError: 'suite_definitions'
Assignee | ||
Comment 34•9 years ago
|
||
Bug 1188330 - Use intree mozharness for taskcluster tests
Assignee | ||
Updated•9 years ago
|
Attachment #8659786 -
Attachment is obsolete: true
Assignee | ||
Comment 35•9 years ago
|
||
Comment on attachment 8662321 [details]
MozReview Request: Bug 1188330 - Use intree mozharness for taskcluster tests
Summarize for this formal patch:
1. Download mozharness.zip only when the url is specified (Although test.yml is only inherited by b2g tests which will have mozharness_url defined).
2. Copy mozharness.zip to artifacts in all b2g build script.
3. Move b2g common configuration, test_packages, to b2g_base.yml
4. And also define mozharness in b2g_base.yml since all b2g build will have mozharness.zip.
5. Remove the old mozharnees variables.
6. Fix the busted in Mulet/B2G_Desktop test, comment #32.
7. Fix the busted in Mulet reftest, comment #33.
Try result for your reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=336e699ff8cf&exclusion_profile=false&group_state=expanded
(I use my own docker hub repository for this try pushing)
Assignee | ||
Comment 36•9 years ago
|
||
Comment on attachment 8662321 [details]
MozReview Request: Bug 1188330 - Use intree mozharness for taskcluster tests
Bug 1188330 - Use intree mozharness for taskcluster tests
Attachment #8662321 -
Flags: review?(wcosta)
Attachment #8662321 -
Flags: review?(garndt)
Attachment #8662321 -
Flags: review?(dustin)
Assignee | ||
Comment 37•9 years ago
|
||
(In reply to Wander Lairson Costa [:wcosta] from comment #26)
> Comment on attachment 8657561 [details] [diff] [review]
> WIP, patch, v1
>
> Review of attachment 8657561 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> lgtm, just a remainder we need to check if it is necessary to modify phone
> tasks too.
BTW, phone tasks don't use mozharness, we don't have to do any thing for phone tasks. Thank you.
Comment 38•9 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #37)
> (In reply to Wander Lairson Costa [:wcosta] from comment #26)
> > Comment on attachment 8657561 [details] [diff] [review]
> > WIP, patch, v1
> >
> > Review of attachment 8657561 [details] [diff] [review]:
> > -----------------------------------------------------------------
> >
> > lgtm, just a remainder we need to check if it is necessary to modify phone
> > tasks too.
>
> BTW, phone tasks don't use mozharness, we don't have to do any thing for
> phone tasks. Thank you.
Phone tasks call the testintg/taskcluster/scripts/phone-builder/build-phone*.sh scripts, which call mozharness.
Flags: needinfo?(echen)
Comment 39•9 years ago
|
||
Comment on attachment 8662321 [details]
MozReview Request: Bug 1188330 - Use intree mozharness for taskcluster tests
https://reviewboard.mozilla.org/r/19575/#review17537
This looks good to me. Just some comments and a suggestion about some cleanup in the mach command either to be done in this bug or a new bug entered to clean it up.
::: testing/docker/tester/bin/entrypoint:15
(Diff revision 1)
> -# used locally in development to test mozharness changes:
> +if [ ! -z "$MOZHARNESS_URL" ]; then
Hrm, is there anytime in a tester when this wouldn't be set? Would it be safter to ensure that it's set to enforce all tasks to have it before doing anything more?
::: testing/mozharness/configs/b2g/mulet_config.py:13
(Diff revision 1)
> - "reftest_options": [
> + "suite_definitions": {
I'm not sure how mozharness handle this, but I assume changing the name of this config option, mozharness knows where to look for things. I see "suite_definitions" used in various configs.
::: testing/mozharness/mozharness/mozilla/testing/gaia_test.py:28
(Diff revision 1)
> -class GaiaTest(TestingMixin, MercurialScript, TransferMixin, GaiaMixin, BlobUploadMixin):
> +class GaiaTest(MercurialScript, TestingMixin, BlobUploadMixin, GaiaMixin, TransferMixin):
did this change because of method resolution?
::: testing/taskcluster/mach_commands.py:405
(Diff revision 1)
> + if 'mozharness' in build_task['task']['extra']['locations']:
nit: At this point it might be simpler to just iterate over all the things in task.extra.locations and add them to some dictionary that is then used to build out the test_parmeters... Doesn't necesasrily have to be changed in this, especially since the current way has been the normal set so far.
location_urls = {}
for location in build_task['task']['extra']['locations']:
location_urls[location] = ARTIFACT_URL.format(
build_parameters['build_slugid'],
build_task['task']['extra']['locations'][location]
for location, url in location_urls.iteritems():
test_parameters['{}_url'.format(location)] = url
Attachment #8662321 -
Flags: review?(garndt) → review+
Comment 40•9 years ago
|
||
Ah, reading the comments, disregard my comments about MRO, suite_definitions, and testing for the mozharness url
Updated•9 years ago
|
Attachment #8662321 -
Flags: review?(dustin) → review+
Comment 41•9 years ago
|
||
Comment on attachment 8662321 [details]
MozReview Request: Bug 1188330 - Use intree mozharness for taskcluster tests
https://reviewboard.mozilla.org/r/19575/#review17551
::: testing/taskcluster/mach_commands.py
(Diff revision 1)
> - 'mozharness_ref':mozharness.get('reference', mozharness['revision']),
I'm happy to see this code going away!
Assignee | ||
Comment 42•9 years ago
|
||
(In reply to Wander Lairson Costa [:wcosta] from comment #38)
> (In reply to Edgar Chen [:edgar][:echen] from comment #37)
> > (In reply to Wander Lairson Costa [:wcosta] from comment #26)
> > > Comment on attachment 8657561 [details] [diff] [review]
> > > WIP, patch, v1
> > >
> > > Review of attachment 8657561 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > >
> > > lgtm, just a remainder we need to check if it is necessary to modify phone
> > > tasks too.
> >
> > BTW, phone tasks don't use mozharness, we don't have to do any thing for
> > phone tasks. Thank you.
>
> Phone tasks call the
> testintg/taskcluster/scripts/phone-builder/build-phone*.sh scripts, which
> call mozharness.
Sorry for didn't make it clear, I meant phone *test* tasks seem not use mozharness. I will double check again.
And you are right, phone build tasks do use mozharness, and most of them has already moved to intree one, except build-dolphin.sh (I will double check this as well) [1]. Since this bug is filed for test tasks, I will file a separated bug for build-dolphin.sh if needed.
Thank you.
[1] https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/scripts/phone-builder/build-dolphin.sh#10-22
Flags: needinfo?(echen)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → echen
Comment 43•9 years ago
|
||
Comment on attachment 8662321 [details]
MozReview Request: Bug 1188330 - Use intree mozharness for taskcluster tests
https://reviewboard.mozilla.org/r/19575/#review17573
::: testing/mozharness/configs/b2g/mulet_config.py:26
(Diff revision 1)
> + },
I also have no knowledge on this part, so I will just trust other reviewers.
::: testing/testsuite-targets.mk:435
(Diff revision 1)
> + cd $(abspath $(PKG_STAGE)) && \
I just assume this part is ok, I have no knowledge on it
Attachment #8662321 -
Flags: review?(wcosta) → review+
Assignee | ||
Comment 44•9 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #42)
> And you are right, phone build tasks do use mozharness, and most of them has
> already moved to intree one, except build-dolphin.sh (I will double check
> this as well) [1]. Since this bug is filed for test tasks, I will file a
> separated bug for build-dolphin.sh if needed.
File bug 1205934 for dolphin build.
>
> Thank you.
>
> [1]
> https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/scripts/
> phone-builder/build-dolphin.sh#10-22
Assignee | ||
Comment 45•9 years ago
|
||
(In reply to Greg Arndt [:garndt] from comment #39)
> Comment on attachment 8662321 [details]
> MozReview Request: Bug 1188330 - Use intree mozharness for taskcluster tests
>
> https://reviewboard.mozilla.org/r/19575/#review17537
>
> ::: testing/taskcluster/mach_commands.py:405
> (Diff revision 1)
> > + if 'mozharness' in build_task['task']['extra']['locations']:
>
> nit: At this point it might be simpler to just iterate over all the things
> in task.extra.locations and add them to some dictionary that is then used to
> build out the test_parmeters... Doesn't necesasrily have to be changed in
> this, especially since the current way has been the normal set so far.
>
> location_urls = {}
> for location in build_task['task']['extra']['locations']:
> location_urls[location] = ARTIFACT_URL.format(
> build_parameters['build_slugid'],
> build_task['task']['extra']['locations'][location]
>
> for location, url in location_urls.iteritems():
> test_parameters['{}_url'.format(location)] = url
Will fire a follow-up bug for this. Thank you.
Assignee | ||
Comment 46•9 years ago
|
||
Hi Wander, I don't have permission to push docker image to taskcluster repository, could you help me to build and push the new docker image based on this patch? Thank you.
Flags: needinfo?(wcosta)
Comment 47•9 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #46)
> Hi Wander, I don't have permission to push docker image to taskcluster
> repository, could you help me to build and push the new docker image based
> on this patch? Thank you.
Redirecting to garndt.
Flags: needinfo?(wcosta) → needinfo?(garndt)
Comment 48•9 years ago
|
||
(In reply to Wander Lairson Costa [:wcosta] from comment #47)
> (In reply to Edgar Chen [:edgar][:echen] from comment #46)
> > Hi Wander, I don't have permission to push docker image to taskcluster
> > repository, could you help me to build and push the new docker image based
> > on this patch? Thank you.
>
> Redirecting to garndt.
Ok, I just pushed the image, could you please test it?
Flags: needinfo?(garndt) → needinfo?(echen)
Assignee | ||
Comment 49•9 years ago
|
||
Comment on attachment 8662321 [details]
MozReview Request: Bug 1188330 - Use intree mozharness for taskcluster tests
Bug 1188330 - Use intree mozharness for taskcluster tests
Assignee | ||
Comment 50•9 years ago
|
||
(In reply to Wander Lairson Costa [:wcosta] from comment #48)
> (In reply to Wander Lairson Costa [:wcosta] from comment #47)
> > (In reply to Edgar Chen [:edgar][:echen] from comment #46)
> > > Hi Wander, I don't have permission to push docker image to taskcluster
> > > repository, could you help me to build and push the new docker image based
> > > on this patch? Thank you.
> >
> > Redirecting to garndt.
>
> Ok, I just pushed the image, could you please test it?
Try result looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=180603ed23cc&group_state=expanded&exclusion_profile=false
Flags: needinfo?(echen)
Comment 52•9 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•