Closed Bug 1238648 Opened 8 years ago Closed 8 years ago

Use external-media-tests from tree instead of github in media-tests mozharness scripts.

Categories

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

Product:
Testing Graveyard
Component:
external-media-tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox47 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: sydpolk, Assigned: sydpolk)

References

Details

Attachments

(1 file)

MozReview Request: Bug 1238648 - Use external-media-tests in tree instead of github for tests. r?maja_zf, r?whimboo
58 bytes, text/x-review-board-request
whimboo
: review+
impossibus
: review+
Details 
When both the mozharness scripts and the tests land in mozilla-central, change mozharness scripts to use mozilla-central.
Depends on: 1239442
Depends on: 1240120
Depends on: 1240991
I think the requirements.txt file in external-media-tests needs to be modified or some conditions have to be set in the mozharness script because it seems like in-tree packages are installed, then uninstalled and replaced by pypi packages.

 22:10:12     INFO -  Installing collected packages: browsermob-proxy, marionette-client, marionette-driver, marionette-transport, firefox-puppeteer, external-media-tests
 22:10:12     INFO -    Found existing installation: browsermob-proxy 0.6.0
 22:10:12     INFO -      Uninstalling browsermob-proxy:
 22:10:12     INFO -        Successfully uninstalled browsermob-proxy
 22:10:12     INFO -    Running setup.py install for browsermob-proxy
 22:10:12     INFO -    Found existing installation: marionette-client 2.0.0
 22:10:12     INFO -      Uninstalling marionette-client:
 22:10:12     INFO -        Successfully uninstalled marionette-client
 22:10:12     INFO -    Running setup.py install for marionette-client
 22:10:12     INFO -      Installing marionette script to /builds/slave/test/build/venv/bin
 22:10:12     INFO -    Found existing installation: marionette-driver 1.1.1
 22:10:12     INFO -      Uninstalling marionette-driver:
 22:10:12     INFO -        Successfully uninstalled marionette-driver
 22:10:12     INFO -    Running setup.py install for marionette-driver
 22:10:13     INFO -    Found existing installation: marionette-transport 1.0.0
 22:10:13     INFO -      Uninstalling marionette-transport:
 22:10:13     INFO -        Successfully uninstalled marionette-transport
 22:10:13     INFO -    Running setup.py install for marionette-transport
 22:10:13     INFO -    Running setup.py install for firefox-puppeteer
 22:10:13     INFO -    Running setup.py install for external-media-tests
Assignee: nobody → spolk
Comment on attachment 8711132 [details]
MozReview Request: Bug 1238648 - Use external-media-tests in tree instead of github for tests. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/31999/#review28953

A few small questions.

::: dom/media/test/external/external-media-tests-requirements.txt:1
(Diff revision 1)
> +-r ../config/marionette_requirements.txt

Just to confirm: you always expect this file to be availabe? Even for mozmill-ci and pf-jenkins jobs?

::: dom/media/test/external/external-media-tests-requirements.txt:2
(Diff revision 1)
> +browsermob-proxy==0.7.1

Is there a particular reason for using this version of browsermob instead of the one specified in marionette_requirements.txt? Could you add a comment to note/explain the choice to replace the in-tree copy? Or maybe write a separate patch to update the in-tree copy of browsermob-proxy instead?

::: testing/mozharness/configs/mediatests/buildbot_windows_config.py:10
(Diff revision 1)
>  config = {
>      "virtualenv_python_dll": 'c:/mozilla-build/python27/python27.dll',
>      "virtualenv_path": 'venv',
>      "exes": {
>          'python': 'c:/mozilla-build/python27/python',
>          'virtualenv': ['c:/mozilla-build/python27/python', 'c:/mozilla-build/buildbotve/virtualenv.py'],
>          'hg': 'c:/mozilla-build/hg/hg',
>          'mozinstall': ['%s/build/venv/scripts/python' % os.getcwd(),
>                         '%s/build/venv/scripts/mozinstall-script.py' % os.getcwd()],
>          'tooltool.py': [sys.executable, 'C:/mozilla-build/tooltool.py'],
> -        'gittool.py': [sys.executable,
> -                       os.path.join(external_tools_path, 'gittool.py')],

This hasn't been tested because win builds were busted due to the upgrade to pip 8. Let's make sure to try again before landing.
Attachment #8711132 - Flags: review?(mjzffr)
Comment on attachment 8711132 [details]
MozReview Request: Bug 1238648 - Use external-media-tests in tree instead of github for tests. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/31999/#review29147

::: dom/media/test/external/external-media-tests-requirements.txt:5
(Diff revision 1)
> +./

Please place this requirements file under /testing/config/ where others already exists. Then you can specify the path to the external media tests package.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py
(Diff revision 1)
> -                                            two_pass=True)

This call is obsolete given that you have this requirement already listed in the above requirements.txt file for external media tests.

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

You may want to keep this method as override and specify the files/directories for extraction from the common.tests.zip file.

See: http://mxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py#192

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:157
(Diff revision 1)
> -                                              'firefox-media-tests')
> +                                              'tests/external-media-tests')

If this is still needed I assume you want to also use os.path.join() for the second line. Don't use a hard-coded slash please.

I for myself don't see why you need this dir listed, at least when applying my above recommendation.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:175
(Diff revision 1)
>                                       'runtests.py')

Maybe you better want to use the package to retrieve the entry script location similar to our fx ui tests:

http://mxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py#295

This would reduce the amount of work in case some locations are getting changed.

::: testing/mozharness/scripts/firefox_media_tests_jenkins.py:29
(Diff revision 1)
>                           'download-and-extract',

If this file is only needed because of using `'read-buildbot-config'` or not, you better define that in the config file and remove duplicated code. mozharness has been created to be mostly config driven, so you might want to make use of it.
Attachment #8711132 - Flags: review?(hskupin)
https://reviewboard.mozilla.org/r/31999/#review29147

> If this file is only needed because of using `'read-buildbot-config'` or not, you better define that in the config file and remove duplicated code. mozharness has been created to be mostly config driven, so you might want to make use of it.

Which file are you talking about?
Comment on attachment 8711132 [details]
MozReview Request: Bug 1238648 - Use external-media-tests in tree instead of github for tests. r?maja_zf, r?whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31999/diff/1-2/
Attachment #8711132 - Flags: review?(mjzffr)
Attachment #8711132 - Flags: review?(hskupin)
Last try run was clean. Here is the latest:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d47f3ff3ff7
Blocks: 1243546
(In reply to Syd Polk :sydpolk from comment #11)
> > If this file is only needed because of using `'read-buildbot-config'` or not, you better define that in the config file and remove duplicated code. mozharness has been created to be mostly config driven, so you might want to make use of it.
> 
> Which file are you talking about?

Both the scripts for buildbot and jenkins. It might be good to see a diff between those two in their current state as of your work on this bug.
Attachment #8711132 - Flags: review?(hskupin) → review+
Comment on attachment 8711132 [details]
MozReview Request: Bug 1238648 - Use external-media-tests in tree instead of github for tests. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/31999/#review29557

That looks great. Only a nit remaining from my side.

::: dom/media/test/external/MANIFEST.in:2
(Diff revision 2)
> +include external-media-tests-requirements.txt

This file got moved, right? I don't think you need that in your package.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:167
(Diff revision 2)
> +                target_unzip_dirs=target_unzip_dirs)

Lovely!
Comment on attachment 8711132 [details]
MozReview Request: Bug 1238648 - Use external-media-tests in tree instead of github for tests. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/31999/#review29579

lgtm
Attachment #8711132 - Flags: review?(mjzffr) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad8793921f4267173157420b6d21e24dd1e3bce
Bug 1238648 - Use external-media-tests in tree instead of github for tests. r=maja_zf, r=whimboo
After doing some investigation, this failure is consistent with the behavior of not having the files of this patch in the common.tests.zip. That zip file will need to be generate before this test will pass.

Bug 1243546 is the next one in line, where these tests will use the in-tree source rather than the tests.zip file.

Could we reapply this patch, generate the common.tests.zip file, and rerun this test?
Flags: needinfo?(spolk) → needinfo?(wkocher)
(In reply to Syd Polk :sydpolk from comment #19)
> Could we reapply this patch, generate the common.tests.zip file, and rerun
> this test?

Don't know anything about any of that stuff you mentioned, but sure.
Flags: needinfo?(wkocher)
I found the problem: - vs _ 
The non-found file is: IOError: Missing files: /builds/slave/test/build/tests/external-media-tests/external-media-tests/playback/youtube/manifest.ini BUT the second 'external-media-tests' should be 'external_media_tests'

This is the breaking change:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad8793921f4267173157420b6d21e24dd1e3bce#l5.171

We could have caught this earlier, but I think b-y got missed in the try syntax somehow. Maybe you need to add something like media-youtube-tests[10.10] to make sure the 10.10 build is included?
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d789a094a8cac8d0aac45cc49ce6d2199438e80
Bug 1238648 - Use external-media-tests in tree instead of github for tests. r=maja_zf, r=whimboo
https://hg.mozilla.org/mozilla-central/rev/1d789a094a8c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: