Add browsermob-proxy options back into the mozharness scripts for external-media-tests

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: sydpolk, Assigned: sydpolk)

Tracking

Version 3
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

These options did not survive the move from the github repository.
Assignee: nobody → spolk
Comment on attachment 8728494 [details]
MozReview Request: Bug 1246271 - The mozharness script at least needs to accept the browsermob settings to pass it down to the test runner. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/38949/#review35625

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:70
(Diff revision 1)
> +      'help': 'port to run the browsermob proxy on',

As :whimboo mentions, setting a default value of None here should shorten the check you have below.
https://reviewboard.mozilla.org/r/38949/#review35625

> As :whimboo mentions, setting a default value of None here should shorten the check you have below.

At some point, you have to convert the port from an int to a string. You should only do that if there is a value for it. Adding a default value of None doesn't change this fact. I chose to do this where the config options are set. I could do it where the command string is generated down into the virtualenv, but I think that's uglier. I will submit my final version for review. I have tested with browsermob options and without.
Comment on attachment 8728494 [details]
MozReview Request: Bug 1246271 - The mozharness script at least needs to accept the browsermob settings to pass it down to the test runner. r?maja_zf, r?whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38949/diff/1-2/
Attachment #8728494 - Flags: review?(mjzffr)
Comment on attachment 8728494 [details]
MozReview Request: Bug 1246271 - The mozharness script at least needs to accept the browsermob settings to pass it down to the test runner. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/38949/#review35641

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:150
(Diff revision 2)
> +        pass

Remove `pass`
Comment on attachment 8728494 [details]
MozReview Request: Bug 1246271 - The mozharness script at least needs to accept the browsermob settings to pass it down to the test runner. r?maja_zf, r?whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38949/diff/2-3/
Attachment #8728494 - Flags: review?(mjzffr)
Comment on attachment 8728494 [details]
MozReview Request: Bug 1246271 - The mozharness script at least needs to accept the browsermob settings to pass it down to the test runner. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/38949/#review35679
Attachment #8728494 - Flags: review?(mjzffr) → review+
Comment on attachment 8728494 [details]
MozReview Request: Bug 1246271 - The mozharness script at least needs to accept the browsermob settings to pass it down to the test runner. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/38949/#review35837

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:69
(Diff revision 3)
> +     {'type': int,

Why do you still force an `int` type here? Keep it as `string` by default. That's anyway what you are using here in that file. The browsermob script should really do a validy check on that argument and not your mozharness script. So I still don't see a reason for such a duplication.

::: testing/mozharness/mozharness/mozilla/testing/firefox_media_tests.py:149
(Diff revision 3)
> +        self.browsermob_port = port

With the above fixed that would be a single line.
Comment on attachment 8728494 [details]
MozReview Request: Bug 1246271 - The mozharness script at least needs to accept the browsermob settings to pass it down to the test runner. r?maja_zf, r?whimboo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38949/diff/3-4/
Attachment #8728494 - Flags: review?(hskupin)
So, the browsermob options in marionette had the port as int, and that is appropriate. For a mozharness script, everything need to stay a string when it is passed down to the actual test process.

I wish mozharness had a way of just pulling out options it needs and pass the rest unfiltered down to the child process.
(In reply to Syd Polk :sydpolk from comment #10)
> I wish mozharness had a way of just pulling out options it needs and pass
> the rest unfiltered down to the child process.

Sadly it doesn't support the mach style, right. Looks like a great bug to be filed, so mozharness can be refactored.
Comment on attachment 8728494 [details]
MozReview Request: Bug 1246271 - The mozharness script at least needs to accept the browsermob settings to pass it down to the test runner. r?maja_zf, r?whimboo

https://reviewboard.mozilla.org/r/38949/#review36163
https://hg.mozilla.org/mozilla-central/rev/940a177ba6a4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.