Closed Bug 1230279 Opened 4 years ago Closed 4 years ago

Develop new mozharness script for use with mozmill-ci jenkins instance.

Categories

(Testing Graveyard :: external-media-tests, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: sydpolk, Assigned: sydpolk)

References

Details

Attachments

(1 file, 3 obsolete files)

mozmill-ci's runtests.py needs to call a mozharness script which can run media tests.
Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo
The jenkins jobs to call this script will be handled with a changes that
need to be made to mozmill-ci, but these changes have to be in place first.
Attachment #8695527 - Flags: review?(mjzffr)
Attachment #8695527 - Flags: review?(hskupin)
Assignee: nobody → spolk
Blocks: 1230342
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review24601

A couple of things which may need to be discussed. Keep in mind that I haven't had a look at media tests yet, so some parts or new to me.

::: testing/mozharness/configs/mediatests/jenkins_config.py:15
(Diff revision 1)
> +    'env': {

Something else which I will add via bug 1230150 is the setting for developer_mode. That means the script is run outside of the releng infrastructure.

::: testing/mozharness/configs/mediatests/jenkins_config.py:45
(Diff revision 1)
> +    'firefox_media_rev': 'b11d6c3d7f6af166be314d2ac6673e78c1edb566',

So you don't want to automatically use the latest revision but explicitely enable a proven rev id? Keep in mind that this will need two steps! First the fix of your config in m-c and then a version bump of mozharness. The latter can not always be enabled in production immediately. So some days of delay will occur.

It would be better to define the repo as default in the module, and specify the branch and revision via the command line as arguments.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:173
(Diff revision 1)
> +        """Overriding method from TestingMixin until firefox-ui-tests are in tree.

until media-tests are in tree.

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:52
(Diff revision 1)
> +                                    'marionette_requirements.txt')

Mind telling me what is special here so that this cannot go into the main module?

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:63
(Diff revision 1)
> +                                                   'blobber_upload_dir')

log files are getting copied to abs_log_dir which is build/upload/logs. It would be better to have all files to upload under that upload folder. So maybe build/upload/blobber?

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:80
(Diff revision 1)
> +                                           'media_tests_mach.log')]

why are those files not uploaded when run via buildbot? IMO this should go into the media module.

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:100
(Diff revision 1)
> +        """ Copy extra (log) files to blob upload dir. """

See my comment above. There is a base method you can use here. See our firefox-ui-tests. Also you might want to have this method checked with the @PostScriptRun decorator. So thta all the files are getting copied at the end and not only if you run this step. Keep in mind that the script can fail before this step!

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:114
(Diff revision 1)
> +            self.move(screenshots_dir, log_screenshots_dir)

self.copy_to_upload_dir() is way better to use.

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:136
(Diff revision 1)
> +                self.copyfile(f, dest)

All that should not be necessary given that the base script is doing it at the end.
Attachment #8695527 - Flags: review?(hskupin)
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review24607

::: testing/mozharness/configs/mediatests/jenkins_config.py:50
(Diff revision 1)
> +    # Default test suite

As in my previous comment on `FirefoxMediaTestsJenkins`, this section was added just for buildbot. You should not have to include it in any Jenkins-specific files and unless you're going to be reusing it for something in mozmill-ci.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:255
(Diff revision 1)
> +        print "Made it to query_minidump_stackwalk"

Are these print statements left over from debugging? Remove them or replace with calls to the logger.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:261
(Diff revision 1)
> +            url_base = 'https://hg.mozilla.org/mozilla-central/raw-file/default/testing/'

I'm not sure `url_base` should always be m-c. For example, what about tests that run against inbound?

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:9
(Diff revision 1)
> +Author: Maja Frydrychowicz

Want to put yourself as the author?

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:34
(Diff revision 1)
> +                         'checkout',

Do we still need the 'checkout' step in Jenkins?

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:52
(Diff revision 1)
> +                                    'marionette_requirements.txt')

Can you point me to which requirements file this is supposed to be? I'd like to take a look at it.

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:82
(Diff revision 1)
> +        test_suite = self.config.get('test_suite')

The `test_suite` and `suite_definitions` stuff here is specific to buildbot so you should be able to remove it.
Attachment #8695527 - Flags: review?(mjzffr)
https://reviewboard.mozilla.org/r/27143/#review24607

> As in my previous comment on `FirefoxMediaTestsJenkins`, this section was added just for buildbot. You should not have to include it in any Jenkins-specific files and unless you're going to be reusing it for something in mozmill-ci.

Right now, the harness uses it to decide which files to run for tests and which to pass in for --urls.

> I'm not sure `url_base` should always be m-c. For example, what about tests that run against inbound?

We could move it into the jenkins_config.py and builtbot_*.py files.

> Do we still need the 'checkout' step in Jenkins?

We need to have both of the firefox-media-tests and firefox-ui-tests repos checked out. Since mozmill-ci's jenkins does not use the git or github plugin, we do need the checkout step. At some point, when we are all in mozilla-central, this won't be needed.
https://reviewboard.mozilla.org/r/27143/#review24601

> Something else which I will add via bug 1230150 is the setting for developer_mode. That means the script is run outside of the releng infrastructure.

Is there anything I need to change in this script as a result of this?

> Mind telling me what is special here so that this cannot go into the main module?

All I know is that this file is at mozilla-central/testing/config/marionette_requirements.txt. It loads mozilla-central/testing/config/mozbase_requirements.txt. Both of these files are references to other directories in either mozilla-central or the tests.zip:

marionette_requirements.txt:
-r mozbase_requirements.txt
../marionette/transport
../marionette/driver
../marionette/marionette/runner/mixins/browsermob-proxy-py
../marionette

mozbase_requirements.txt:
../mozbase/manifestparser
../mozbase/mozcrash
../mozbase/mozdebug
../mozbase/mozdevice
../mozbase/mozfile
../mozbase/mozhttpd
../mozbase/mozinfo
../mozbase/mozinstall
../mozbase/mozleak
../mozbase/mozlog
../mozbase/moznetwork
../mozbase/mozprocess
../mozbase/mozprofile
../mozbase/mozrunner
../mozbase/mozscreenshot
../mozbase/moztest
../mozbase/mozversion

I am not sure which requirements I need to compare these to?

> log files are getting copied to abs_log_dir which is build/upload/logs. It would be better to have all files to upload under that upload folder. So maybe build/upload/blobber?

We are following what firefox_ui_tests.py does. https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py#189
https://reviewboard.mozilla.org/r/27143/#review24607

> Can you point me to which requirements file this is supposed to be? I'd like to take a look at it.

mozilla-central/testing/config/marionette_requirements.txt

https://dxr.mozilla.org/mozilla-central/source/testing/config/marionette_requirements.txt

> The `test_suite` and `suite_definitions` stuff here is specific to buildbot so you should be able to remove it.

Actually, this is used to distinguish which url files and manifest files to pass in. Saves command line arguments, since mozmill-ci's jobs don't normally pass in test-specific arguments; they are in configuration files.
https://reviewboard.mozilla.org/r/27143/#review24607

> We could move it into the jenkins_config.py and builtbot_*.py files.

This will be taken care of when we move everything to the tree.
https://reviewboard.mozilla.org/r/27143/#review24601

> Is there anything I need to change in this script as a result of this?

You can run firefox-media-tests directory outside of any infrastructure. You can also run the firefox_media_tests.py mozharness script outside of releng infrastructure.
Bug 1230279 - Further refactoring to enable mozmill-ci mozharness script for media tests; r?maja_zf, whimboo
Attachment #8697578 - Flags: review?(mjzffr)
Attachment #8697578 - Flags: review?(hskupin)
The try build is failing because buildbot doesn't know about the new script. So you have to make this change in 3 steps:

1. Add the new entry script and leave the old one
2. Update buildbot-config for the new script: http://hg.mozilla.org/build/buildbot-configs/file/tip/mozilla-tests/config.py#l411
3. Remove the old script
Bug 1230279 - Leave link to new firefox_media_tests_buildbot.py in old position until buildbot config can be updated.
Bug 1230279 - Leave symlink at mediatests for firefox_media_tests in config directory until buildbot-configs can be fixed.
Blocks: 1232026
https://reviewboard.mozilla.org/r/27641/#review25115

::: testing/mozharness/configs/firefox_media_tests/jenkins_config.py:51
(Diff revision 1)
>      'suite_definitions': {

Do you need to add more suite definitions for the other tests that will run in Jenkins? Long-running tests? EME tests?

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:9
(Diff revision 1)
> -Author: Maja Frydrychowicz
> +Author: Syd Polk

This change should only be made to firefox_media_tests_jenkins.py.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:184
(Diff revision 1)
>      def download_and_extract(self):

Without having seen a try run, I suspect this will break firefox_media_tests_buildbot.py. Perhaps it should only be in the Jenkins-specific script.

::: testing/mozharness/scripts/firefox_media_tests/firefox_media_tests_jenkins.py:67
(Diff revision 1)
>          test_suite = self.config.get('test_suite')

firefox_media_tests_buildbot.py relies on the `test_suite` stuff, too. This piece should therefore more to firefox_media_tests.py.

Thanks, Syd, almost there, I think. :) Please flag me for review again once you have a green try run.
https://reviewboard.mozilla.org/r/27143/#review25117

I believe I already reviewed this revision. Mozreview being weird, maybe?
https://reviewboard.mozilla.org/r/27641/#review25161

Looks way cleaner meanwhile. I have added some comments which need an update of the code or a discussion to happen.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:9
(Diff revision 1)
> -Author: Maja Frydrychowicz
> +Author: Syd Polk

Usually we do not add authors given that this information is already given via the history of the file.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:184
(Diff revision 1)
>      def download_and_extract(self):

Yeah. Simply taking this method will not work. Once our tests are in the tree I will also have to update / remove this method so it picks up the base behavior.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:205
(Diff revision 1)
> +        dirs['abs_test_install_dir'] = os.path.join(dirs['abs_work_dir'],

Is `abs_test_install_dir` what the default `download_and_extract` methods sets? If not you may want to follow the base prop name.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:261
(Diff revision 1)
> +        cmd += ['--log-mach', os.path.join(log_dir, 'media_tests_mach.log')]

IMHO the name of the html and mach files could be better chosen. We know already that those files are for media tests.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:288
(Diff revision 1)
>          super(FirefoxMediaTestsBase, self).query_minidump_stackwalk(manifest=manifest_path)

I missed to add the return statement here, which I fixed already. You should also do it, otherwise your crash will never be examined.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:299
(Diff revision 1)
> +        log_screenshots_dir = os.path.join(log_dir, 'screenshots')

Personally I don't think that screenshots should go into the log folder. But that's your both decision.
Depends on: 1234577
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27143/diff/1-2/
Attachment #8695527 - Flags: review?(mjzffr)
Attachment #8695527 - Flags: review?(hskupin)
Attachment #8697578 - Attachment is obsolete: true
Attachment #8697578 - Flags: review?(hskupin)
Attachment #8697647 - Attachment is obsolete: true
Attachment #8697653 - Attachment is obsolete: true
Current try run passed:

https://reviewboard.mozilla.org/r/27141
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27143/diff/2-3/
This patch is finally ready for final approval.

Last try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d16de7f45ad8
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review26017

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:181
(Diff revisions 2 - 3)
> +

Go ahead and update the buildbot configs under testing/mozharness/configs/mediatests to only refer to the submodule-free revision of firefox-media-tests and then remove all references to firefox-ui-tests in all the mediatest scripts.
Attachment #8695527 - Flags: review?(mjzffr)
https://reviewboard.mozilla.org/r/27143/#review24601

> So you don't want to automatically use the latest revision but explicitely enable a proven rev id? Keep in mind that this will need two steps! First the fix of your config in m-c and then a version bump of mozharness. The latter can not always be enabled in production immediately. So some days of delay will occur.
> 
> It would be better to define the repo as default in the module, and specify the branch and revision via the command line as arguments.

The `checkout` step is going away in a few weeks once the tests move in-tree, so I wouldn't bother changing this.
Argh, the above comment is from a few weeks ago. I thought it was published in mozreview but apparently it wasn't. I also now realize that a bunch of other comments I left a few weeks ago in Mozreview appear to be missing. Will try to find them or reproduce them now.
https://reviewboard.mozilla.org/r/27143/#review26021

::: testing/mozharness/scripts/firefox_media_tests/firefox_media_tests_buildbot.py:7
(Diff revision 3)
> +"""firefox_media_tests_buildbot.py

I don't think you should copy/move firefox_media_tests_buildbot.py to scripts/firefox_media_tests. Moving the script  requires changes in the buildbot config in https://hg.mozilla.org/build and introduces complications (different buildbot config for m-c vs m-a). At the very least this type of change should be handled in a separate bug. In any case, I'm not sure it's worth the trouble given that buildbot won't be around that much longer.

::: testing/mozharness/scripts/firefox_media_tests/firefox_media_tests_jenkins.py:76
(Diff revision 3)
> +        if test_suite not in self.config["suite_definitions"]:

Everything related to 'test_suite' should be in the base script (firefox_media_tests.py) since both the jenkins and the buildbot script will use it. (See bug 1209327)

::: testing/mozharness/scripts/firefox_media_tests_buildbot.py:31
(Diff revision 3)
>  buildbot_media_test_options = [

This should move to the base script (firefox_media_tests.py) as well.

::: testing/mozharness/scripts/firefox_media_tests_buildbot.py
(Diff revision 3)
> -        test_manifest = None if test_suite != 'media-youtube-tests' else \

Don't remove the 'media-youtube-tests' stuff from the buildbot script.
You can push your patch to cedar to see if it works well with media-youtube-tests (b-y). (e.g. https://treeherder.mozilla.org/#/jobs?repo=cedar&filter-job_type_symbol=b-y)
https://reviewboard.mozilla.org/r/27143/#review26021

> Don't remove the 'media-youtube-tests' stuff from the buildbot script.

Why not the base script? Jenkins needs it as well.
Attachment #8695527 - Flags: review?(mjzffr)
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27143/diff/3-4/
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review26069

::: testing/mozharness/scripts/firefox_media_tests_buildbot.py
(Diff revision 4)
> -        config_options = copy.deepcopy(blobupload_config_options)

You need `blobupload_config_options` for BlobUploadMixin to work. I believe this is why the b-y job failed on cedar.
Attachment #8695527 - Flags: review?(mjzffr)
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27143/diff/4-5/
Attachment #8695527 - Flags: review?(mjzffr)
https://reviewboard.mozilla.org/r/27143/#review26021

> Why not the base script? Jenkins needs it as well.

If it's in the base script then that's fine.
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review26085

::: testing/mozharness/configs/firefox_media_tests/buildbot_posix_config.py:1
(Diff revision 5)
> +import os

As discussed, let's keep all the configs in the mediatests directory, rather than creating firefox_media_tests. This is to avoid unnecessary changes to RelEng's buildbot config.

For consistency, you might want to change scripts/firefox_media_tests to scripts/mediatests as well.

::: testing/mozharness/scripts/firefox_media_tests/firefox_media_tests_jenkins.py:18
(Diff revision 5)
> +from mozharness.base.log import ERROR, DEBUG, INFO

There are various unused imports to clean up here and elsewhere.
Attachment #8695527 - Flags: review?(mjzffr)
https://reviewboard.mozilla.org/r/27143/#review26085

> As discussed, let's keep all the configs in the mediatests directory, rather than creating firefox_media_tests. This is to avoid unnecessary changes to RelEng's buildbot config.
> 
> For consistency, you might want to change scripts/firefox_media_tests to scripts/mediatests as well.

I don't want to change the name at this point. Changing "firefox_media_tests_buildbot.py" would force us to revise buildbot-configs. As far as mozilla/testing/firefox_media_tests.py, there is a firefox_ui_tests.py in that same directory...
Attachment #8695527 - Flags: review?(mjzffr)
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27143/diff/5-6/
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review26517

I skimmed over those scripts and found a couple of issues and raised some questions. Keep in mind that I'm still not that familiar with the code so Maja should have the final saying about the review.

::: testing/mozharness/configs/mediatests/jenkins_config.py:61
(Diff revision 6)
> +    hgtool = os.path.join(external_tools_path, 'hgtool.py')

I don't see why you need this else case. `sys.executable` works also fine on Windows.

So simply copy what we have:
http://mxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/firefox_ui_tests/releng_release.py#20

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:155
(Diff revision 6)
> +        self.test_packages_url = c.get('test_packages_url')

You will also need `self.test_url` here. See my update for the mozharness patch which just landed for firefox-ui-tests.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:165
(Diff revision 6)
> +                                            two_pass=True)

What's the `two_pass` part here? Is that necessary? At least for our ui tests we don't need it.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:190
(Diff revision 6)
> -        abs_dirs = super(FirefoxMediaTestsBase, self).query_abs_dirs()
> +        abs_dirs = VCSToolsScript.query_abs_dirs(self)

Why this change? As you told me you should keep using `super`.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:213
(Diff revision 6)
> +            self.firefox_media_vc['revision'] = c['firefox_media_rev']

You could also use `c.get('firefox_media_rev', 'default')` and get rid of the if condition.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:216
(Diff revision 6)
>          revision = self.vcs_checkout(vcs='gittool', **self.firefox_media_vc)

`revision` is not used anymore and can be removed.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:251
(Diff revision 6)
> +            self.fatal("%s is not defined in the config!" % test_suite)

I think you should put this line before the above code on line 242.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:286
(Diff revision 6)
> +        super(FirefoxMediaTestsBase, self).query_minidump_stackwalk(manifest=manifest_path)

You are still not returning a value here.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:297
(Diff revision 6)
> +        log_screenshots_dir = os.path.join(log_dir, 'screenshots')

IMO screenshots are not logs and should end-up in a differnt folder. But that's up to you.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:339
(Diff revision 6)
> -        if (not os.environ.get('MINIDUMP_STACKWALK') and
> +        if self.query_minidump_stackwalk():

See my comment above. This method does not return a value yet. So this will always be False right now.

::: testing/mozharness/scripts/firefox_media_tests_buildbot.py
(Diff revision 6)
> -        self.test_packages_url = c.get('test_packages_url')

Interesting that you needed this for buildbot. AFAIK the `read-buildbot-config` step should set most of these values.

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:46
(Diff revision 6)
> +        super(FirefoxMediaTestsJenkins, self)._pre_create_virtualenv(action)

Hm, how do you access this requirements file if you don't use the test package via jenkins? I feel something is suspicious here.

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:57
(Diff revision 6)
> +        return self.abs_dirs

Can't those extra definitions not be part of the base class `query_abs_dir` method?
https://reviewboard.mozilla.org/r/27143/#review26517

> I don't see why you need this else case. `sys.executable` works also fine on Windows.
> 
> So simply copy what we have:
> http://mxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/firefox_ui_tests/releng_release.py#20

So, when I use that definition of config['exes'] and run it on the Mac, I get this:

15:59:49    ERROR - Exception during pre-action for checkout: Traceback (most recent call last):
15:59:49    ERROR -   File "/Users/spolk/dev/mozilla/mozilla-central/testing/mozharness/mozharness/base/script.py", line 1638, in run_action
15:59:49    ERROR -     method(action)
15:59:49    ERROR -   File "/Users/spolk/dev/mozilla/mozilla-central/testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py", line 203, in _pre_checkout
15:59:49    ERROR -     super(FirefoxMediaTestsBase, self)._pre_checkout(action)
15:59:49    ERROR -   File "/Users/spolk/dev/mozilla/mozilla-central/testing/mozharness/mozharness/mozilla/vcstools.py", line 45, in _pre_checkout
15:59:49    ERROR -     file_path = self.which(vcs_tool)
15:59:49    ERROR -   File "/Users/spolk/dev/mozilla/mozilla-central/testing/mozharness/mozharness/base/script.py", line 788, in which
15:59:49    ERROR -     if self.is_exe(exe):
15:59:49    ERROR -   File "/Users/spolk/dev/mozilla/mozilla-central/testing/mozharness/mozharness/base/script.py", line 766, in is_exe
15:59:49    ERROR -     return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
15:59:49    ERROR -   File "/usr/local/Cellar/python/2.7.10/Frameworks/Python.framework/Versions/2.7/lib/python2.7/genericpath.py", line 37, in isfile
15:59:49    ERROR -     st = os.stat(path)

This is why this has to be a conditional.

After discussing this in irc, we agreed that the conditional is fine for now.

> IMO screenshots are not logs and should end-up in a differnt folder. But that's up to you.

We can revisit this later. Going to leave it as is for now.

> Interesting that you needed this for buildbot. AFAIK the `read-buildbot-config` step should set most of these values.

It's been a while, but when I did not have this, things went south quickly.

> Hm, how do you access this requirements file if you don't use the test package via jenkins? I feel something is suspicious here.

It's in the mozharness/testing/config directory. Seems to work on my desktop. If this turns out to be a problem, we will fix it when we move it to mozmill-ci.
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27143/diff/6-7/
Attachment #8695527 - Flags: review?(hskupin)
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review26705

Overall I noticed a couple of small bugs and a few opportunities to avoid repetition. The most important problem is that log uploads are broken on the media buildbot jobs. 

Some open issues seem to have been resolved in your latest revision but were not marked as such: help a poor reviewer out and update all the issues accordingly. :) Thanks!

::: testing/mozharness/configs/mediatests/buildbot_windows_config.py:22
(Diff revision 7)
>          'hgtool.py': [sys.executable,

Optional: hgtool.py can be removed. (The posix config doesn't haven't. I probably forgot to remove it when I submitted my initial patch.)

::: testing/mozharness/configs/mediatests/buildbot_windows_config.py:55
(Diff revision 7)
> -    "firefox_media_rev": '49b500b30b80372a6c678ec7d0a2b074844f5e84',
> +    "firefox_media_rev": '453276b8d056fedba45aacd6b02021e7cc637fc2',

Optional: you can update this to the latest revision. It's not crucial. 00ab0ee356955722ccac32f5d831755f4331a4fc

If you go ahead with this, please also follow-up on my related comment in firefox_media_tests.py.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:7
(Diff revision 7)
>  """firefox_media_tests.py

We commented on this in an earlier revision but it must have been missed: lines 7-10 can be removed.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:161
(Diff revision 7)
> +        requirements = os.path.join(dirs['abs_test_install_dir'],
> +                                    'config',
> +                                    'marionette_requirements.txt')
> +        if os.access(requirements, os.F_OK):
> +            self.register_virtualenv_module(requirements=[requirements])
> +
>          requirements_file = os.path.join(dirs['firefox_media_dir'],
>                                           'requirements.txt')
> +
>          if os.path.isfile(requirements_file):
>              self.register_virtualenv_module(requirements=[requirements_file])

This section does the same thing twice, in an inconsistent way, with marionette_requirements.txt and requirements.txt. Let's clean it up.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:173
(Diff revision 7)
>          self.register_virtualenv_module(name='firefox-media-tests',

If you decide to update the media test revision in the buildbot configs, as suggested in my other comment, then I think you should be able to remove this line because the latest version of the requirements file on Github takes care of the 'firefox-media-tests' package. I'm not 100% sure, so do test it out.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:212
(Diff revision 7)
> +            'firefox_media_rev' : c.get('firefox_media_rev', 'default')

This dictionary item has to be called 'revision', not 'firefox_media_rev' in order to match the arguments expected by self.vcs_checkout; i.e. change `'firefox_media_rev': c.get('firefox_media_rev',...)` to `'revision': c.get('firefox_media_rev',...)`.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:326
(Diff revision 7)
> +            self.copy_to_upload_dir(os.path.join(dirs['abs_log_dir'], f),

I think this change may have broken something with log uploads on buildbot. Your try runs for b-m show no uploads in the Job details pane on TH, and the latest log says "WARNING - Blob upload directory does not exist!". You might need to add a different version of this method in the buildbot script.

::: testing/mozharness/scripts/firefox_media_tests_buildbot.py:47
(Diff revision 7)
>      @PreScriptAction('create-virtualenv')

This is now taken care of in firefox_media_tests.py so you can remove it.

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:38
(Diff revision 7)
> +    def _pre_create_virtualenv(self, action):

Since this is already done in the base script and can be removed, and since the jenkins script doesn't really do anything else, it looks like you can just remove it altogether! Instead, add a "default_actions" item to the jenkins config file. (See example in the buildbot config files.)
Attachment #8695527 - Flags: review?(mjzffr)
https://reviewboard.mozilla.org/r/27143/#review26517

> What's the `two_pass` part here? Is that necessary? At least for our ui tests we don't need it.

This was originally cargo-culted from https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/marionette.py; if it doesn't hurt, it might be safer to keep it.

> Can't those extra definitions not be part of the base class `query_abs_dir` method?

I suspect anything related to blob_upload is only relevant to buildbot, so maybe not everything should move into the base class.
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

I will wait with my review until all the changes Maja mentioned have been implemented.
Attachment #8695527 - Flags: review?(hskupin)
Just putting in some information so it won't get lost:

1. The very long tests may stay on pf-jenkins.
2. We may still have to run Netflix tests on pf-jenkins unless we find a good solution for the profiles with valid logins.
3. These changes to mozilla-central will ride the trains, so we will have to wait to turn off the aurora tests on pf-jenkins.
The pf-jenkins jobs will need to get their source from hg after the move is complete.
https://reviewboard.mozilla.org/r/27143/#review26705

> Optional: hgtool.py can be removed. (The posix config doesn't haven't. I probably forgot to remove it when I submitted my initial patch.)

Why was it there in the first place?

> We commented on this in an earlier revision but it must have been missed: lines 7-10 can be removed.

The only comment I saw was to change the author name to myself. I can remove this.

> This section does the same thing twice, in an inconsistent way, with marionette_requirements.txt and requirements.txt. Let's clean it up.

What would be the correct thing to do here?
https://reviewboard.mozilla.org/r/27143/#review26705

> Why was it there in the first place?

I answered this on IRC: bad copy-paste probably?

> Optional: you can update this to the latest revision. It's not crucial. 00ab0ee356955722ccac32f5d831755f4331a4fc
> 
> If you go ahead with this, please also follow-up on my related comment in firefox_media_tests.py.

Note that Bug 1237774 will land soon.

> What would be the correct thing to do here?

As discussed on IRC: `requirements` vs `requirements_file`; `os.access(...)` vs `os.path.isfile(...)`.
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27143/diff/7-8/
Attachment #8695527 - Flags: review?(mjzffr)
Attachment #8695527 - Flags: review?(hskupin)
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review27037

I will let Maja give you the final r+. While skimming over those changes I noticed the following issue. Not sure when and if it will break your runs. But it could after new releases went out.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:166
(Diff revision 8)
>                                           'requirements.txt')

Something I missed earlier. This can cause dependency conflicts. You should better use all packages from the test package OR from PyPI, but not a mix of both.
Blocks: 1238629
Blocks: 1238648
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review27065

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py
(Diff revisions 7 - 8)
> -        # configure logging
> -        log_dir = dirs.get('abs_log_dir')
> -        cmd += ['--gecko-log', os.path.join(log_dir, 'gecko.log')]
> -        cmd += ['--log-html', os.path.join(log_dir, 'media_tests.html')]
> -        cmd += ['--log-mach', os.path.join(log_dir, 'media_tests_mach.log')]

You probably want to move this '#configure logging' stuff into the jenkins script instead of removing it altogether.

::: testing/mozharness/scripts/firefox_media_tests_buildbot.py:81
(Diff revisions 7 - 8)
>      def _collect_uploads(self, action, success=None):
>          """ Copy extra (log) files to blob upload dir. """
>          dirs = self.query_abs_dirs()
>          log_dir = dirs.get('abs_log_dir')
>          blob_upload_dir = dirs.get('abs_blob_upload_dir')
>          if not log_dir or not blob_upload_dir:

Something is missing here. `_collect_uploads` doesn't really do anything. How do the media-tests screenshots get uploaded?
https://reviewboard.mozilla.org/r/27143/#review26705

> Since this is already done in the base script and can be removed, and since the jenkins script doesn't really do anything else, it looks like you can just remove it altogether! Instead, add a "default_actions" item to the jenkins config file. (See example in the buildbot config files.)

I'm reopening this issue because there's still a copy of this method in the jenkins script.
https://reviewboard.mozilla.org/r/27143/#review27037

> Something I missed earlier. This can cause dependency conflicts. You should better use all packages from the test package OR from PyPI, but not a mix of both.

As discussed in Vidyo chat, this will be addressed in another bug, bug 1238629.
https://reviewboard.mozilla.org/r/27143/#review26705

> I'm reopening this issue because there's still a copy of this method in the jenkins script.

Um, it's not in my version.
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27143/diff/8-9/
Attachment #8695527 - Flags: review?(mjzffr)
Attachment #8695527 - Flags: review?(hskupin)
Attachment #8695527 - Flags: feedback+
Here is a try run for media-youtube-tests (try: -b o -p macosx64 -u media-youtube-tests[10.10] -t none[10.10])

https://treeherder.mozilla.org/#/jobs?repo=try&revision=701e0b2df389
So, this try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db7369e4cc42&selectedJob=15310689

has a successful windows run of b-m on Windows 8 x64 debug, and the correct artifacts are showing.

There are two b-m runs still pending on Windows 8 x64 opt; these are very slow to get started right now. I don't think that our tests have anything to do with them.
https://reviewboard.mozilla.org/r/27143/#review27065

> Something is missing here. `_collect_uploads` doesn't really do anything. How do the media-tests screenshots get uploaded?

This is in my version as well.
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

https://reviewboard.mozilla.org/r/27143/#review27373

Huzzah!
Attachment #8695527 - Flags: review?(mjzffr) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

I wonder why my mozreview comment and approval didn't made it to the bug. r=me
Attachment #8695527 - Flags: review?(hskupin) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6f207cecddef
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
This needs to be uplifted to aurora for bug 1237179 to work correctly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

Approval Request Comment
[Feature/regressing bug #]: 1237179
[User impact if declined]: Firefox media tests will fail on Aurora. (Treeherder group VP)
[Describe test coverage new/current, TreeHerder]: This is a testing only fix. Firefox media tests are Tier 2 now anyway; having them fail is not a release blocker.
[Risks and why]: Low - this could keep already broken tests broken. 
[String/UUID change made/needed]: none
Attachment #8695527 - Flags: approval-mozilla-aurora?
Comment on attachment 8695527 [details]
MozReview Request: Bug 1230279 - Develop new mozharness script for media-tests for Jenkins; r?maja_zf, whimboo

We don't need this uploaded after all.
Attachment #8695527 - Flags: approval-mozilla-aurora?
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.