Closed Bug 1188330 Opened 4 years ago Closed 4 years ago

Use intree mozharness for taskcluster tests

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

defect
Not set

Tracking

(firefox43 fixed, b2g-master affected)

RESOLVED FIXED
Tracking Status
firefox43 --- fixed
b2g-master --- affected

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)
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/
Blocks: 1188335
Blocks: 1188336
Blocks: 1188337
Blocks: 1188338
We should also clean up mozilla-taskcluster not to populate the MOZHARNESS_* environment variables in the generated task definition...
(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
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
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1185643
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
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.
Flags: needinfo?(jlund)
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.
reopening in agreement with comment 6 and since this isn't done yet :)
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
blocking-b2g: --- → 2.5+
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
double checking 1181261 isn't involved via https://bugzilla.mozilla.org/show_bug.cgi?id=1181261#c29
Hi Jonathan,
Can you find someone who can help here? This bug is the major blocker for emulator on TC.
Thanks.
Flags: needinfo?(jgriffin)
(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)
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+ → ---
(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?
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.
(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)
(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)
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.
> 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
Attached patch WIP, patch, v1 (obsolete) — Splinter Review
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 on attachment 8657561 [details] [diff] [review]
WIP, patch, v1

Adding :garndt for feedback.
Attachment #8657561 - Flags: feedback?(garndt)
Most of the tests failed, but I don't know if it is related to the patch.
(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 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 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 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 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+
(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.
(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
Summary: Use intree mozharness for Mulet Gb, Gbu, Gu and Li → Use intree mozharness for taskcluster tests
Attached patch WIP, Patch, v2 (obsolete) — Splinter Review
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
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'
Bug 1188330 - Use intree mozharness for taskcluster tests
Attachment #8659786 - Attachment is obsolete: true
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)
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)
(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.
(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 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+
Ah, reading the comments, disregard my comments about MRO, suite_definitions, and testing for the mozharness url
Attachment #8662321 - Flags: review?(dustin) → review+
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!
(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: nobody → echen
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+
(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
(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.
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)
(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)
(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)
Comment on attachment 8662321 [details]
MozReview Request: Bug 1188330 - Use intree mozharness for taskcluster tests

Bug 1188330 - Use intree mozharness for taskcluster tests
(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)
https://hg.mozilla.org/mozilla-central/rev/7f2c1a5e6771
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.