Closed Bug 1243546 Opened 4 years ago Closed 4 years ago

The external media tests should be invokeable via mach

Categories

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

Version 3
defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: sydpolk, Assigned: sydpolk)

References

Details

Attachments

(1 file)

external-media-tests should be runnable from mozilla-central via mach. This should use the built firefox and the source tree for the tests.
Is there an existent mach command for media tests? If yes, that one could be extended via a flag.
The only media tests run right now are via mochitest or via webplatform-tests. I will be adding a separate command.
Attachment #8715073 - Flags: review?(hskupin)
Attachment #8715073 - Flags: review?(gps)
Assignee: nobody → spolk
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

This patch does not contain the mach commands for external media tests but simply a copy of firefox-ui-tests. Maybe you missed a final commit? Please fix that and re-request review.
Attachment #8715073 - Flags: review?(hskupin)
Attachment #8715073 - Flags: review?(gps)
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33327/diff/1-2/
Attachment #8715073 - Attachment description: MozReview Request: Bug 1243546 - The external media tests should be invokeable via mach. → MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?whimboo, gps
Attachment #8715073 - Flags: review?(hskupin)
Attachment #8715073 - Flags: review?(gps)
Attachment #8715073 - Flags: review?(gps)
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

https://reviewboard.mozilla.org/r/33327/#review30181

::: dom/media/test/external/mach_commands.py:20
(Diff revision 2)
> +from external_media_harness.runtests import (
> +    FirefoxMediaHarness,
> +    MediaTestArguments,
> +    MediaTestArgumentsBase,
> +    MediaTestRunner,
> +    mn_cli,
> +)

We try not to globally import non-stdlib modules in mach_commands.py files because it can lead to import bloat and slow down `mach` invocations. Please perform the imports in the methods that need them so the import penalty is deferred until actual use.
Attachment #8715073 - Flags: review?(gps)
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33327/diff/2-3/
Please also see bug 1245468 for mentioned issues with the firefox-ui-tests mach command. You might not want to simply copy and paste our code. Please check if anything of that also applies to your command.
I am following bug 1245468.
I would rather not hold up having the ./mach external-media-tests being available waiting for bug 1235468 to be resolved. I tried some of the suggestions in that issue, and found that changes to the underlying mozharness script are going to be necessary. I did fix one bug that caused problems in argument processing, and need to examine the same issue in firefox-ui-tests.

As changes to testing/firefox-ui/mach_command.py happen, I will examine them and make necessary changes to dom/media/test/external/mach_command.py.
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33327/diff/3-4/
Attachment #8715073 - Attachment description: MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?whimboo, gps → MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps
Attachment #8715073 - Flags: review?(hskupin)
Attachment #8715073 - Flags: feedback?(hskupin)
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

https://reviewboard.mozilla.org/r/33327/#review30391

Looks fine to me. I added some questions because I wonder about some lines. Maybe you can explain?

::: dom/media/test/external/mach_commands.py:45
(Diff revision 4)
> +        args.binary = kwargs['binary']

I wonder when this is necessary given that this differs from fx-ui-tests mach command.

::: dom/media/test/external/mach_commands.py:46
(Diff revision 4)
> +    path, exe = os.path.split(args.binary)

Both variables are not used. So this line can be removed. I will have to do the same for the firefox-ui-tests mach command.

::: dom/media/test/external/mach_commands.py:49
(Diff revision 4)
> +        setattr(args, k, v)

I wonder if `args.update(kwargs)` would work here.
Attachment #8715073 - Flags: review+
https://reviewboard.mozilla.org/r/33327/#review30391

> I wonder when this is necessary given that this differs from fx-ui-tests mach command.

If I did not do this, the update to kwargs['binary'] in the calling function for some reason was not being propogated, and my script was complaining that there was no --binary set.
https://reviewboard.mozilla.org/r/33327/#review30391

> I wonder if `args.update(kwargs)` would work here.

Nope.

AttributeError: 'Namespace' object has no attribute 'update'

  File "/Users/spolk/dev/mozilla/mozilla-central/dom/media/test/external/mach_commands.py", line 72, in run_external_media_test
    return run_external_media_test(tests, topsrcdir=self.topsrcdir, **kwargs)
  File "/Users/spolk/dev/mozilla/mozilla-central/dom/media/test/external/mach_commands.py", line 47, in run_external_media_test
    args.update(kwargs)
Attachment #8715073 - Flags: feedback?(hskupin)
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33327/diff/4-5/
Gregory, could you review this as soon as you can get to it? I have addressed your initial concerns. Thanks.
Flags: needinfo?(gps)
Attachment #8715073 - Flags: review?(gps) → review+
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

https://reviewboard.mozilla.org/r/33327/#review30949

LGTM
Flags: needinfo?(gps)
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/be13802a57ee - apparently this is the week for OS X "NameError: global name 'args' is not defined" errors like https://treeherder.mozilla.org/logviewer.html#?job_id=21380883&repo=mozilla-inbound, yours is the second one I've seen.
Oh, in this case Windows agrees that it's not defined, we just hadn't gotten around to running any VP on Windows yet.
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

https://reviewboard.mozilla.org/r/33327/#review31027

::: dom/media/test/external/external_media_harness/runtests.py:96
(Diff revision 5)
> +           args=args)

So the problem is here... args is not defined.
Attachment #8715073 - Flags: review+
Comment on attachment 8715073 [details]
MozReview Request: Bug 1243546 - Add mach command for external-media-tests - r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33327/diff/5-6/
Gregory, this got backed out. Could you put your stamp of approval on it again? Thanks.
Flags: needinfo?(gps)
Turns out I was wrong about the review flag being cleared. This should be OK now.
Flags: needinfo?(gps)
https://hg.mozilla.org/mozilla-central/rev/94e09f808ad1
Status: NEW → RESOLVED
Closed: 4 years ago
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.