1235606 - Move firefox-media-tests from github to mozilla-central

Status: RESOLVED FIXED

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

Description

[Syd Polk :sydpolk]

The Firefox media tests need to be in mozilla-central. They will be put into mozilla-central/testing/firefox-media.

Comment 1

[Syd Polk :sydpolk]

Attachment: MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

Review commit: https://reviewboard.mozilla.org/r/29179/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29179/

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

I can't review this, but here's some quick feedback:
- remove the LICENSE file
- remove all references to pf-jenkins in the readme
- ask David and/or Henrik for feedback as well

I'm not clear on whether this is a temporary location for these media tests: will they be moved to live with other media tests later on? (under dom/media?) See https://bugzilla.mozilla.org/show_bug.cgi?id=1212609#c11 for similar discussion about firefox-ui-tests.

Comment 3

[Syd Polk :sydpolk]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f16278caf954

Comment 4

[Syd Polk :sydpolk]

Updated try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3285cbaf73aa

Comment 5

[Syd Polk :sydpolk]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29179/diff/1-2/

Comment 6

[Syd Polk :sydpolk]

Ted, is the location I am proposing for these tests OK? Or do we need to put them somewhere in the media tree?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I think the location is fine for the harness, but if your tests are not getting packaged (something we have to do for now for our firefox-ui-tests) then you might really want to put them into the media component next to its other tests.

I did a quick skim over the patch and I feel that you should really separate the harness from the tests. That's just my 2c but Ted could give all the details.

Comment 8

[Syd Polk :sydpolk]

I am moving this to dom/media/tests/media-element. I am not sure how much more separate the harness needs to be; it would be nice to be able to run the tests directly from the source directory.

Comment 9

[Syd Polk :sydpolk]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29179/diff/2-3/

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

https://reviewboard.mozilla.org/r/29179/#review26475

I'm happy with tests being in dom/media/. Will leave review of python/marionette stuff to others more qualified.

::: dom/media/test/media_element/MANIFEST.in:1
(Diff revision 3)
> +exclude MANIFEST.in

Sticking this in dom/media/test/$something/ is appropriate, but I'd pref $something to not be "media_element", and the name implies it's unit tests, but this is really integration tests that test whether important third party external sites keep working.

Can we rename it to something that captures that, for example "external-media-sites"?

::: dom/media/test/media_element/README.md:1
(Diff revision 3)
> +firefox-media-tests

You should comment in here somewhere that a signed plugin-container.exe and voucher.bin is required for the EME tests to work. i.e. you can't './mach build' and expect the EME tests to Just Work.

Comment 11

[(not currently active) Ted Mielczarek]

(In reply to Syd Polk :sydpolk from comment #8)
> I am moving this to dom/media/tests/media-element. I am not sure how much
> more separate the harness needs to be; it would be nice to be able to run
> the tests directly from the source directory.

That sounds fine. There's some precedent here, the reftest harness lives in layout/tools/reftest. I think in an ideal world the harness code would live under testing/ and the tests would live next to the code they test, and `mach test <path to tests>` would do the right thing, but you can start here and iterate.

Comment 12

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

https://reviewboard.mozilla.org/r/29179/#review26569

::: dom/media/test/media_element/README.md:3
(Diff revision 3)
> +[![Code Health](https://landscape.io/github/mjzffr/firefox-media-tests/master/landscape.png)](https://landscape.io/github/mjzffr/firefox-media-tests/master)

Remove the landscape.io line.

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

The test code and related libraries have already been reviewed on Github; this is just a move from Github to hg.m.o and the proposed move looks good to me.

Comment 14

[Syd Polk :sydpolk]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29179/diff/3-4/

Comment 15

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/29179/#review26601

::: dom/media/test/external-media-tests/requirements.txt:4
(Diff revision 4)
> +mozdevice==0.46

requirements.txt got updated today - please include that in your patch: https://github.com/mjzffr/firefox-media-tests/blob/master/requirements.txt

I noticed that the copy under media-element is still there -- you want to remove that, right?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

https://reviewboard.mozilla.org/r/29179/#review26737

Why do we only land the tests and not combined with the harness? Means this will add non-working tests to mozilla-central.

Otherwise I roughly skimmed the patch and added some notes. Keep in mind that I'm not a peer of media and cannot give a review. So f+

::: dom/media/test/external-media-tests/MANIFEST.in:5
(Diff revision 4)
> +recursive-include media_utils *

You may also want to make the tests not a Python package. But that's not a blocker for now, and can be done as a follow-up similar to the firefox-ui-tests.

::: dom/media/test/external-media-tests/setup.py:14
(Diff revision 4)
> +    'firefox_puppeteer == 3.0.0',

We updated the package yesterday. You may want to use firefox-puppeteer from now on. Keep in mind that this does not affect the module itself. Just the package name.

Comment 17

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://reviewboard.mozilla.org/r/29179/#review26737

> We updated the package yesterday. You may want to use firefox-puppeteer from now on. Keep in mind that this does not affect the module itself. Just the package name.

I'll update this in the Github repo now and Syd can just grab the latest revision for this move.

Comment 18

[Syd Polk :sydpolk]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29179/diff/4-5/

Comment 19

[Syd Polk :sydpolk]

All review comments have been addressed.

Comment 20

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

One last thing: include the update from bug 1237774.

Also you should probably change your commit message to refer only to cpearce, not me and whimboo.

Comment 21

[Syd Polk :sydpolk]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29179/diff/5-6/

Comment 22

[Syd Polk :sydpolk]

I have addressed all review comments, and merged in the very latest from the firefox-media-tests repo. As far as I can tell, this is ready go go.

Chris, Ted, is this OK?

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

https://reviewboard.mozilla.org/r/29179/#review26921

This is fine with me.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://reviewboard.mozilla.org/r/29179/#review26737

I haven't seen a reply yet to this question.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://reviewboard.mozilla.org/r/29179/#review27039

I checked the github repository for mediatests and I still feel that sub folder media_test_harness and media_utils are missing here.

Comment 26

[Syd Polk :sydpolk]

(In reply to Henrik Skupin (:whimboo) from comment #25)
> https://reviewboard.mozilla.org/r/29179/#review27039
> 
> I checked the github repository for mediatests and I still feel that sub
> folder media_test_harness and media_utils are missing here.

firefox-media-tests github repo is unchanged. The subdirectories in this patch are called "harness", "media_tests", and "media_utils". I do not plan on changing the github repo any further.

Comment 27

[Syd Polk :sydpolk]

This is ready to go.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8702746 [details]
MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce

We talked about the remaining issue in our vidyo chat and Syd filed a follow-up for that. So all fine from my side too.

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Syd Polk :sydpolk from comment #22)
> I have addressed all review comments, and merged in the very latest from the
> firefox-media-tests repo. As far as I can tell, this is ready go go.
> 
> Chris, Ted, is this OK?

Yep.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a342a8d66c0d

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This bug is not fixed yet. It will be marked as fixed once the code has been landed on mozilla-central.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a342a8d66c0d