Closed Bug 1235606 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: sydpolk, Assigned: sydpolk)

References

Details

Attachments

(1 file)

The Firefox media tests need to be in mozilla-central. They will be put into mozilla-central/testing/firefox-media.
Assignee: nobody → spolk
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.
Attachment #8702746 - Flags: review?(mjzffr)
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/
Attachment #8702746 - Flags: review?(mjzffr)
Ted, is the location I am proposing for these tests OK? Or do we need to put them somewhere in the media tree?
Flags: needinfo?(ted)
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.
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.
Attachment #8702746 - Attachment description: MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, maja_zf → MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?cpearce, r?maja_zf
Attachment #8702746 - Flags: review?(mjzffr)
Attachment #8702746 - Flags: review?(cpearce)
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/
Attachment #8702746 - Flags: review?(cpearce) → review+
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.
(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.
Flags: needinfo?(ted)
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.
Attachment #8702746 - Flags: review?(mjzffr)
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 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/
Attachment #8702746 - Attachment description: MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?cpearce, r?maja_zf → MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?maja_zf, r?ted, r?whimboo
Attachment #8702746 - Flags: review?(mjzffr)
Attachment #8702746 - Flags: review?(hskupin)
Attachment #8702746 - Flags: review?(mjzffr) → feedback+
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?
Attachment #8702746 - Flags: review?(hskupin)
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.
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.
Blocks: 1237767
Attachment #8702746 - Flags: review?(mjzffr)
Attachment #8702746 - Flags: review?(hskupin)
Attachment #8702746 - Flags: feedback+
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/
All review comments have been addressed.
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.
Attachment #8702746 - Flags: review?(mjzffr)
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/
Attachment #8702746 - Attachment description: MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?maja_zf, r?ted, r?whimboo → MozReview Request: Bug 1235606 - Move firefox-media-tests into mozilla-central. r?ted, r?cpearce
Attachment #8702746 - Flags: review?(hskupin)
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?
Attachment #8702746 - Flags: feedback?(mjzffr)
Attachment #8702746 - Flags: feedback?(hskupin)
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.
Attachment #8702746 - Flags: review+
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.
Attachment #8702746 - Flags: feedback?(mjzffr) → feedback+
Blocks: 1238627
(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.
This is ready to go.
Keywords: checkin-needed
Blocks: 1238641
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.
Attachment #8702746 - Flags: feedback?(hskupin) → feedback+
Blocks: 1238648
(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.
Blocks: 1197234
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This bug is not fixed yet. It will be marked as fixed once the code has been landed on mozilla-central.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/a342a8d66c0d
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.