Test TURN server for mochitest in CI

RESOLVED FIXED in Firefox 49

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

(Depends on: 1 bug)

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

58 bytes, text/x-review-board-request
drno
: review+
ahal
: review+
Details | Review
1.36 KB, text/x-python
ahal
: feedback+
Details
58 bytes, text/x-review-board-request
drno
: review+
Details | Review
58 bytes, text/x-review-board-request
ahal
: review+
Details | Review
58 bytes, text/x-review-board-request
ahal
: review+
Details | Review
58 bytes, text/x-review-board-request
gps
: review+
Details | Review
58 bytes, text/x-review-board-request
ahal
: review+
Details | Review
(Assignee)

Description

2 years ago
Building on bug 1231977 (probably), we need to build a test TURN server.
Rank: 15
(Assignee)

Comment 1

2 years ago
Created attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review commit: https://reviewboard.mozilla.org/r/36405/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36405/
Attachment #8723196 - Flags: review?(drno)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/1-2/
Attachment #8723196 - Attachment description: MozReview Request: Bug 1231981: Very basic test STUN server for running in CI. r?drno → MozReview Request: Bug 1231981: Very basic test TURN server for running in CI. r?drno
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://reviewboard.mozilla.org/r/36405/#review33103

::: testing/tools/iceServer/iceServer.py:49
(Diff revision 2)
> +        result = (result << 8) + byte

Not sure if we need to worry about overflows on 32bit Python here?
If I understand it correct that could be prevented by explicitly making |result| of type long.

::: testing/tools/iceServer/iceServer.py:62
(Diff revision 2)
> +    lastBit = startBit - numBits + 1

What happens if lastBit turns negative, because numBits > (startBit +1)?

::: testing/tools/iceServer/iceServer.py:97
(Diff revision 2)
> +        length = parseInt(buf[2:4])

Not sure, but it might be helpful to store the parse length in the instance for future reference.

::: testing/tools/iceServer/iceServer.py:102
(Diff revision 2)
> +        self.data = buf[self.__attrHeaderSize:self.__attrHeaderSize+length]
> +
> +        while length % 4:
> +            length += 1 # ignore padding

Not sure I like the idea that the parser on purpose ignores remaining bytes in the buffer beyond the current attribute.

::: testing/tools/iceServer/iceServer.py:104
(Diff revision 2)
> +        while length % 4:
> +            length += 1 # ignore padding

In a strict mode we should raise an exception if we find something else then zero's in the padding area.

::: testing/tools/iceServer/iceServer.py:115
(Diff revision 2)
> +        self.cookie = 0x2112A442

The magic cookie value should probably go up into the list of global constants, or is there any scenario where a StunMessage instance modifies this value?

::: testing/tools/iceServer/iceServer.py:144
(Diff revision 2)
> +        buf = buf[0:minBufSize]

Are you ignoring trailing bytes here on purpose?
Depending on the type of testing a strict mode which complains or rejects these kind of problems might be helpful.

::: testing/tools/iceServer/iceServer.py:219
(Diff revision 2)
> +            return None

This should raise an Exception. Otherwise you just get strange errors when trying to serializeInt(None).

::: testing/tools/iceServer/iceServer.py:233
(Diff revision 2)
> +            return

Exception?

::: testing/tools/iceServer/iceServer.py:266
(Diff revision 2)
> +            return None

Exception please

::: testing/tools/iceServer/iceServer.py:335
(Diff revision 2)
> +class UdpNetworkHandler(NetworkHandler):

Just curious: are we limited in using third party libraries when using Python code in CI, or what else motivated you to avoid using something like twisted for the networking parts?

::: testing/tools/iceServer/iceServer.py:407
(Diff revision 2)
> +        UdpNetworkHandler.__init__(self, makeUdpSocket((v4Address, 0), socket.AF_INET))

Is this on purpose limited to v4?

::: testing/tools/iceServer/iceServer.py:554
(Diff revision 2)
> +# TODO: Firefox isn't setting up permissions for all the stuff it sends
> +# indications to. Looking at the spec, this is a bug

Is this "just" the fact that we send only a one shot permissions request right before the data indication, or something else?

::: testing/tools/iceServer/iceServer.py:581
(Diff revision 2)
> +        response.addNonce("foobajoobaaa")

Can we make this random instead?

::: testing/tools/iceServer/iceServer.py:623
(Diff revision 2)
> +turnUser="foo"
> +turnPass="bar"
> +turnRealm="mozilla.invalid"

I guess we make this later configurable

::: testing/tools/iceServer/iceServer.py:639
(Diff revision 2)
> +    dummySocket = socket.socket(socket.AF_INET6, socket.SOCK_DGRAM)

Not sure if we have to worry about hosts in CI w/o IPv6 setup?

::: testing/tools/iceServer/iceServer.py:651
(Diff revision 2)
> +    print ("iceServers:[{username:\"" + turnUser + "\",credential:\"" + turnPass + "\",url:\"turn:localhost\"}," +

How about announcing as a STUN server as well?
I think once this lands we should test with all possible combinations.

::: testing/tools/iceServer/iceServer.py:654
(Diff revision 2)
> +    while True:

This doesn't strike me as the easiest way to get later extended with different behavior on different port numbers. But we can probably re-factor that later.

::: testing/tools/iceServer/iceServer.py:655
(Diff revision 2)
> +        read, write, excpt = select.select(servers, filter(NetworkHandler.hasWrites, servers), servers, 0.1)

select? Yikes. Well I guess this is not going to be high performance, high throughput any way...
Attachment #8723196 - Flags: review?(drno)
(Assignee)

Comment 4

2 years ago
https://reviewboard.mozilla.org/r/36405/#review33103

> Not sure if we need to worry about overflows on 32bit Python here?
> If I understand it correct that could be prevented by explicitly making |result| of type long.

From what I read, Python automatically converts to long as needed. Empirically, I have seen no problem with unpacking of transaction ids.

> What happens if lastBit turns negative, because numBits > (startBit +1)?

I can validate this.

> Not sure I like the idea that the parser on purpose ignores remaining bytes in the buffer beyond the current attribute.

We could run into problems verifying message integrity or fingerprint if something padded with something other than zeroes. But, firefox doesn't do this, so we should be ok.

> In a strict mode we should raise an exception if we find something else then zero's in the padding area.

Sure, why not. That'll guard against the problem I just described.

> The magic cookie value should probably go up into the list of global constants, or is there any scenario where a StunMessage instance modifies this value?

I'll make it a constant.

> Are you ignoring trailing bytes here on purpose?
> Depending on the type of testing a strict mode which complains or rejects these kind of problems might be helpful.

Yes, because this might have come over TCP.

> This should raise an Exception. Otherwise you just get strange errors when trying to serializeInt(None).

Sure.

> Exception?

Sure.

> Exception please

Sure.

> Just curious: are we limited in using third party libraries when using Python code in CI, or what else motivated you to avoid using something like twisted for the networking parts?

I blew a half-hour trying to figure out how to get the source ip/port out of Twisted, with no luck. SocketServer was a little better, but made it very difficult to get send buffering to work at all, and even without that I had to do a lot of monkeying around in the internals/overriding stuff/using the UDPServer to handle TCP traffic. Once everything was done, I decided it was ugly enough to abandon.

> Is this on purpose limited to v4?

As there is no way to request a V6 allocation in the base spec, yes. Maybe at some point it would be worth implementing RFC 6156.

> Is this "just" the fact that we send only a one shot permissions request right before the data indication, or something else?

This comment was mistaken. Have removed in my working copy. The permission code was wrong (permissions ignore port).

> Can we make this random instead?

Yeah, why not.

> I guess we make this later configurable

Or we could randomize, since it outputs the necessary iceServers:, I guess it depends.

> Not sure if we have to worry about hosts in CI w/o IPv6 setup?

I'll wrap this in a try.

> How about announcing as a STUN server as well?
> I think once this lands we should test with all possible combinations.

Probably makes sense. The codepaths for STUN are different.

> This doesn't strike me as the easiest way to get later extended with different behavior on different port numbers. But we can probably re-factor that later.

Not sure what you mean here.

> select? Yikes. Well I guess this is not going to be high performance, high throughput any way...

Is there a faster way to do this in python?
(Assignee)

Comment 5

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/2-3/
Attachment #8723196 - Flags: review?(drno)
(Assignee)

Comment 6

2 years ago
So, where is going to be the best place to run this thing? It is probably best to run/kill once per webrtc mochitest (can we use the .ini tags for this, maybe?).
Flags: needinfo?(catlee)
(Assignee)

Comment 7

2 years ago
Also, who do I ask for review on the testing/automation side of things? ted?
(Assignee)

Updated

2 years ago
Assignee: nobody → docfaraday
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://reviewboard.mozilla.org/r/36405/#review33561

LGTM
Attachment #8723196 - Flags: review?(drno) → review+
https://reviewboard.mozilla.org/r/36405/#review33103

> Not sure what you mean here.

Once we are happy with having this up and running as a properly working STUN/TURN server I imagine that we are going to want special instances of this which are mis-behaving on purpose, like delay answers, no answer for X packets, don't answer over UDP etc. And this endless loop in main doesn't make that easier. But as I said that is easy enough to refactor later.

> Is there a faster way to do this in python?

I was hoping for poll(), but the poll() implementation apparently depends on the OS, thus you would need an abstraction layer to figure out what is supported by the host OS. Probably not worth writing something like that right now if we don't get it for free through a lib like Twisted.
(In reply to Byron Campen [:bwc] from comment #6)
> So, where is going to be the best place to run this thing? It is probably
> best to run/kill once per webrtc mochitest (can we use the .ini tags for
> this, maybe?).

My guess is that we would need to start it before our CPP unit tests start, and also before our mochitests start, right?
And we will tear it down once our tests are over. So the question is basically how can we start and stop something as test fixtures?
(Assignee)

Comment 11

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/3-4/
So to make sure I understand.. You want to run some python code before and after a specific test or group of tests. Unfortunately there's not a great way to do that.

Likely you'll need to use message passing over the structured logger. Grep for "buffering_on" under testing/mochitest to see an example of a pre-existing custom message like this. A big caveat, is that the logger can't pass messages from python back to JS. So the tests would need to do some sort of polling to figure out when the server is ready/shutdown.

Another alternative is to roll your own websocket based protocol and just have the harness kick start the server process at the beginning. This option might be easier if you have to do this for several different harnesses.
Flags: needinfo?(catlee)
(Assignee)

Comment 13

2 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #12)
> So to make sure I understand.. You want to run some python code before and
> after a specific test or group of tests. Unfortunately there's not a great
> way to do that.

   We'd want to launch a python process before a set of tests, and then kill that process once the tests are done.

> Likely you'll need to use message passing over the structured logger. Grep
> for "buffering_on" under testing/mochitest to see an example of a
> pre-existing custom message like this. A big caveat, is that the logger
> can't pass messages from python back to JS. So the tests would need to do
> some sort of polling to figure out when the server is ready/shutdown.
>
> Another alternative is to roll your own websocket based protocol and just
> have the harness kick start the server process at the beginning. This option
> might be easier if you have to do this for several different harnesses.


   That seems a little more involved than I was hoping; would it be easier if we ran the server at the beginning of the entire suite, and killed at the end, but only if the suite contains a test tagged 'webrtc'?
Flags: needinfo?(ahalberstadt)
I guess we could do that.. I'd sort of prefer not to though. Plus your proposal may cause intermittents in the other tests and end up being even more work in the long run (don't underestimate how flaky mochitests are :p). Passing messages via the logger isn't terribly hard assuming you can poll for the server from the test. Is that no possible?

Alternatively we could make the webrtc tests into their own flavor or subsuite, then only run the server if we are running that job. They'd have their own unique symbol in treeherder that way. Though this takes a bit of effort as well.
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 15

2 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #14)
> I guess we could do that.. I'd sort of prefer not to though. Plus your
> proposal may cause intermittents in the other tests and end up being even
> more work in the long run (don't underestimate how flaky mochitests are :p).
> Passing messages via the logger isn't terribly hard assuming you can poll
> for the server from the test. Is that no possible?

   To be honest, I'm not wild about (ab)using the structured logger to launch processes. It would also be really nice to be able to have some two-way communication (the TURN server spits out some JSON that I would like to use to configure stuff in the test-case, ideally, and it would be very nice to have any output from the TURN server in the logging).

> 
> Alternatively we could make the webrtc tests into their own flavor or
> subsuite, then only run the server if we are running that job. They'd have
> their own unique symbol in treeherder that way. Though this takes a bit of
> effort as well.

   This seems somewhat promising, but does it give us the console output from the TURN server?

   You mentioned the idea of a custom websocket dealie earlier; are you talking about a really tiny python websocket server that is run at the beginning of the test suite, and when we open a websocket to it, it runs the TURN server and pipes us the output?
From my point of view once we have the TURN server integrated we will write a lot of tests (or change existing tests) which will have a dependency on the server being available.
To me the natural solution for such a dependency of tests is that the test framework provides a setup function. And if any error, like failing to start a server, results in the tests getting skipped or the test getting marked as failed (test result depends on preferences).

Now by letting the logging framework start something I don't see how the test would know if the required server is actually up and running. That smells to like intermittent tests results (depending on reliably we can start the server, and if starting the server and test timing wise works out).

You could now require that every single test which has a dependency on external services needs to check for their availability, but that is IMHO just pushing the job of test framework down into the tests and will result in lots of duplicated code.

I understand that most teams in Mozilla, and especially not teams with code in mozilla central, don't have such external dependencies. But would still be possible to extend our test frameworks to handle these things in a sane way like most other test frameworks I have seen would do it (e.g. have a [Setup] section in the .ini file)?
We can also offer our support in writing this, as long as such effort is not going to be rejected once we try to land it.

BTW this does not only affect mochitest. I think we have an even bigger desire to have the TURN server available for our CPP unit tests.
A lot of test directories have a sort of setup/teardown between tests, but it's all in JS and isn't built-in to the harness (usually these files are called head.js). I would love for the harness to be able to support what you want, your efforts definitely wouldn't be rejected. This would actually be my preferred solution. I didn't suggest this approach because it is the most difficult path, and I wasn't sure who would have the resources to work on it. I'm happy to provide guidance and reviews though if this was something you wanted to try.

With that being said, here's how mochitest works (I'm less familiar with CPP):
1. INI manifests are processed and converted into a list of tests
2. Python process starts gecko
3. A dedicated thread processes output (i.e the structured logs)
4. Main thread blocks until Gecko shuts down

Your problem basically boils down to how to communicate from JS to Python. Currently stdout is the only way to do this, but this is uni-directional. A better way would be over sockets (I think there's actually a bug on file somewhere to send the logs over a socket rather than over stdout). No matter how we slice it (whether we use manifests, or build something into the harness, or include a setup file next to the tests), this will be the biggest technical hurdle.
To highlight my point a bit more clearly..

The python side is dumb. It just launches Gecko and then waits for it to shutdown again. It pushes the tests down a hill and is powerless to do anything about them until they roll to a stop.
(Assignee)

Comment 19

2 years ago
So, let's talk about using a python websocket server to launch the TURN server and bridge stuff like stdout.

It looks like the harness code is already running a websocket server in python for the duration of the test suite; how hard would it be to teach this to do the following when a specific URL (say, /turnserver) is hit?

1. Launch the TURN server.
2. Send all stdout/stderr to the websocket client.
3. Ignore writes from the websocket client (we _could_ send this to stdin, but unless we have a good reason I think we should avoid this)
4. When the websocket is closed, kill the process.
5. If the process ends, close the websocket (after flushing writes)

If it would be hard to add this functionality to the existing websocket server, I'm pretty sure I can whip up something purpose-built with twisted pretty quickly.
Flags: needinfo?(ahalberstadt)
We could possibly subvert the existing websocket server, but I don't know much about it, or if it would have an impact on existing tests. The other downside to this approach is that it wouldn't be very portable to e.g CPP unittests if that's something you wanted.

To clarify, is this server going to be started once at the start and left running for the duration of the webrtc tests? If so, then maybe we should just spin off a new mochitest flavor and have the harness start it up as part of the setup. I can help with this if we go this route.

Another question.. Are you using mochitest out of necessity or out of familiarity? It might also be worth looking into using the marionette test suite instead. Those tests are written in python (though they can execute arbitrary JS in either chrome or content) and have proper setUp and tearDown methods between each test. It would definitely be a lot easier to start/stop a python server with those.
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 21

2 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #20)
> We could possibly subvert the existing websocket server, but I don't know
> much about it, or if it would have an impact on existing tests. The other
> downside to this approach is that it wouldn't be very portable to e.g CPP
> unittests if that's something you wanted.

   Good point.

> 
> To clarify, is this server going to be started once at the start and left
> running for the duration of the webrtc tests? If so, then maybe we should
> just spin off a new mochitest flavor and have the harness start it up as
> part of the setup. I can help with this if we go this route.
> 

   Yes, this is the intent. A new mochitest flavor is appealing in some ways, I guess it depends on how hard it is to do. FWIW, this websocket server is very simple and lightweight (I can attach it).

> Another question.. Are you using mochitest out of necessity or out of
> familiarity? It might also be worth looking into using the marionette test
> suite instead. Those tests are written in python (though they can execute
> arbitrary JS in either chrome or content) and have proper setUp and tearDown
> methods between each test. It would definitely be a lot easier to start/stop
> a python server with those.

   I am unsure, as I have not worked with marionette. I suspect that we've just never had much reason to switch, since we have no real need for driving the UI.
(Assignee)

Comment 22

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/4-5/
(Assignee)

Comment 23

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Re-asking review, rewrote using twisted, and a few other changes.
Attachment #8723196 - Flags: review+ → review?(drno)
(Assignee)

Comment 24

2 years ago
Created attachment 8726761 [details]
Simple websocket-based ICE server launcher
Attachment #8726761 - Flags: feedback?(ahalberstadt)
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://reviewboard.mozilla.org/r/36405/#review34783

::: testing/tools/iceServer/iceServer.py:384
(Diff revisions 4 - 5)
> -        return origLen - len(data)
> +    lastindication = None

What is this?
Doesn't seem to be used any where. If you need it I would move it into the self. space up in __init__()

::: testing/tools/iceServer/iceServer.py:676
(Diff revisions 4 - 5)
> -            "{url:\"stun:localhost\"}," +
> +    allocationPruner.start(1)

Shouldn't be sufficient to run the pruning every 5 or 10 seconds?
Attachment #8723196 - Flags: review?(drno) → review+
(Assignee)

Comment 26

2 years ago
https://reviewboard.mozilla.org/r/36405/#review34783

> What is this?
> Doesn't seem to be used any where. If you need it I would move it into the self. space up in __init__()

That's dead code, thanks for spotting.

> Shouldn't be sufficient to run the pruning every 5 or 10 seconds?

I was thinking that we'd want fairly fast expiration in case we wanted to test it, although we could get away with checking the expiry whenever something tries to use the allocation, and a less frequent pruning loop.
(Assignee)

Comment 27

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/5-6/
Attachment #8723196 - Attachment description: MozReview Request: Bug 1231981: Very basic test TURN server for running in CI. r?drno → MozReview Request: Bug 1231981: Very basic test TURN server for running in CI. r=drno
Comment on attachment 8726761 [details]
Simple websocket-based ICE server launcher

This looks reasonable to me, though I'm not familiar with twisted, so can't say whether it's accurate. This could potentially go in testing/tools as place it can be re-used. The dependency on twisted will also need to be dealt with.

I think it would still be easier to spin off a new flavor or subsuite and launch the ICE server when that is run.
Attachment #8726761 - Flags: feedback?(ahalberstadt) → feedback+
(In reply to Andrew Halberstadt [:ahal] from comment #28)
> I think it would still be easier to spin off a new flavor or subsuite and
> launch the ICE server when that is run.

Sounds good. But I assume a new flavor would only cover the mochitests, right?

Maybe we can take care of the CPP unit test part in separate ticket, but as we so far seem to run all CPP unit tests in one big chunk I'm assuming that we are not going to split the WebRTC CPP unit tests out. Therefore we would still need a solution to start the dependency for that. And I guess some socket based control would/could be still a good solution.
Ah yes, that's a good point. CPP doesn't have a concept of flavors or subsuites.. so it would be significantly more work to do that there.
(Assignee)

Comment 31

2 years ago
Using a websocket thingie wouldn't help the CPP (or gecko gtest) tests anyway. So what needs to be done to create a new mochitest flavor? Will running a single test-case manually run the fixture too?
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 32

2 years ago
Before we lose track of it, the websocket thing does offer some advantages:

- A crash in the TURN server is detected easily by the test code (the websocket drops)
- The mochitest gets the output from the TURN server (useful because the first thing it outputs is the iceServers config to use)
- The mochitest could send commands to the TURN server; this could be useful later on.
- Each mochitest gets a pristine TURN server; could cut down on oranges.
Though it might be good to make a new flavor/subsuite either way, so we can run the websocket thing only when necessary. I think for this case a subsuite is our best bet. Here's how they work.

1. Add a subsuite key to every test and/or DEFAULT section in a manifest that you want to be included in this subsuite. Doing this automatically removes the test from the default set (it'll no longer be run as part of regular mochitests). E.g:
https://dxr.mozilla.org/mozilla-central/source/dom/push/test/mochitest.ini#2

2. Somewhere in the mochitest harness, add code that runs either the TURN server or the websocket thing behind an "if options.subsuite == 'webrtc'" block.

3. Create a new job (in taskcluster or buildbot) that passes in --subsuite=webrtc, and give it a unique symbol (possibly 'wr'?)

4. Running locally should *just work*. Tests in this subsuite should automatically be picked up if a containing path is used:
./mach mochitest path/to/dir/containing/webrtc/tests

Alternatively the entire subsuite can be run with:
./mach mochitest --flavor plain --subsuite webrtc
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 34

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/6-7/
Attachment #8723196 - Attachment description: MozReview Request: Bug 1231981: Very basic test TURN server for running in CI. r=drno → MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno
(Assignee)

Comment 35

2 years ago
Created attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review commit: https://reviewboard.mozilla.org/r/39525/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39525/
(Assignee)

Comment 36

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Is this roughly what we're looking for?
Attachment #8729628 - Flags: feedback?(drno)
Attachment #8729628 - Flags: feedback?(ahalberstadt)
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Yeah! I only looked at the stuff under testing/mochitest, but it looks great.
Attachment #8729628 - Flags: feedback?(ahalberstadt) → feedback+
(Assignee)

Comment 38

2 years ago
Ok, I have some cleanup to do, and can then put it up for review on Monday. What needs to be done to get our python dependencies in place?
Flags: needinfo?(ahalberstadt)
Are the dependencies just the python files for the server? Typically it's just copying files to the relevant directories in the objdir. You can do this with the TEST_HARNESS_FILES variable in moz.build.
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 40

2 years ago
I was referring to stuff like twisted.
(Assignee)

Comment 41

2 years ago
Also, what do I do to get these new subsuites running in CI? I'm guessing I would need to touch stuff somewhere in h.m.o/build? I see buildbot stuff, but my understanding is that things are migrating to taskcluster; should I still be using the buildbot-configs repo?
(In reply to Byron Campen [:bwc] from comment #40)
> I was referring to stuff like twisted.

Right, that's a bit more tricky. Because the test machines can't hit the network, it needs to be uploaded to an internal pypi server:
http://puppetagain.pub.build.mozilla.org/data/python/packages/

I think you need to file bug under Release::Engineering and ask someone to do it (ping #releng for more info). Then we'll need to update the mozharness script to install twisted into the virtualenv that the test machines use from that pypi server. For the local path we can get mach to install twisted from the official pypi if it isn't available.

An alternative is to check twisted into the tree under /python.. but we try to avoid doing that unless it's a package that's so ubiquitous it can be useful everywhere.


(In reply to Byron Campen [:bwc] from comment #41)
> Also, what do I do to get these new subsuites running in CI? I'm guessing I
> would need to touch stuff somewhere in h.m.o/build? I see buildbot stuff,
> but my understanding is that things are migrating to taskcluster; should I
> still be using the buildbot-configs repo?

Yes, things are migrating to taskcluster, but very slowly. If you only need to run this on linux64 you can ignore buildbot. Otherwise you'll need to deal with buildbot-configs and taskcluster. See the buildbot-configs/mozilla-tests/config.py file. It's kind of horrible, but there are examples you can copy. Try to follow the MOCHITEST_PUSH config as an example (these tests are variants of mochitest-plain right?).
(Assignee)

Updated

2 years ago
Depends on: 1256676
(Assignee)

Comment 43

2 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #42)
> (In reply to Byron Campen [:bwc] from comment #40)
> > I was referring to stuff like twisted.
> 
> Right, that's a bit more tricky. Because the test machines can't hit the
> network, it needs to be uploaded to an internal pypi server:
> http://puppetagain.pub.build.mozilla.org/data/python/packages/

  Hey, twisted seems to be there already. There are some other things we would need though. I've opened a bug.

> I think you need to file bug under Release::Engineering and ask someone to
> do it (ping #releng for more info). Then we'll need to update the mozharness
> script to install twisted into the virtualenv that the test machines use
> from that pypi server. For the local path we can get mach to install twisted
> from the official pypi if it isn't available.

  What files need to be touched here?

> An alternative is to check twisted into the tree under /python.. but we try
> to avoid doing that unless it's a package that's so ubiquitous it can be
> useful everywhere.

   We have a few more things than twisted, so we probably don't want to do this.

> (In reply to Byron Campen [:bwc] from comment #41)
> > Also, what do I do to get these new subsuites running in CI? I'm guessing I
> > would need to touch stuff somewhere in h.m.o/build? I see buildbot stuff,
> > but my understanding is that things are migrating to taskcluster; should I
> > still be using the buildbot-configs repo?
> 
> Yes, things are migrating to taskcluster, but very slowly. If you only need
> to run this on linux64 you can ignore buildbot. Otherwise you'll need to
> deal with buildbot-configs and taskcluster. See the
> buildbot-configs/mozilla-tests/config.py file. It's kind of horrible, but
> there are examples you can copy. Try to follow the MOCHITEST_PUSH config as
> an example (these tests are variants of mochitest-plain right?).

   We need this across the board, and from my limited look around there's going to be a _lot_ of stuff that needs to be touched. Further, is it even possible to test changes in buildbot-configs on try? This is looking like an awful lot of trouble to me. I can see the value in having our own mochitest flavor, since that will make it really easy to select our tests on try, but perhaps that's something that we should put off until taskcluster is fully deployed? If it makes it easier to swallow, I can modify the websocket doodad that I wrote to be more extensible, suitable for a general purpose launcher, so other test suites can use it if necessary.
(Assignee)

Comment 44

2 years ago
I should note that mochitest-push isn't run on all platforms, so it is an incomplete example.
How about we land your TURN server code here. And then tackle the remaining CI problems in new tickets?
(Assignee)

Comment 46

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/7-8/
(Assignee)

Comment 47

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/1-2/
Attachment #8729628 - Flags: feedback?(drno)
Attachment #8729628 - Flags: feedback+
(Assignee)

Comment 48

2 years ago
Created attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

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

Not a full review. Just the pieces I noticed when trying to understand the new Turn server startup logic.

::: dom/media/tests/mochitest/pc.js:1808
(Diff revision 2)
> +  var enableHttpProxy = enable => new Promise(resolve => {
> +    SpecialPowers.pushPrefEnv(
> +        {'set': [['media.peerconnection.disable_http_proxy', !enable]]},
> +        resolve);
> +  });

I would prefer if you would simply resolve the promise right away if disabling the http proxy is not needed.
That way you can skip the SpecialPowers.pushPrefEnv() call and just run with the default value.

::: dom/media/tests/mochitest/pc.js:1853
(Diff revision 2)
>        startNetworkAndTest()
> +        .then(() => setupIceServerConfig(fixtureOptions.useIceServer))

Wouldn't it make sense to roll setupIceServerConfig() into the startNetworkAndTest()?
I would then rename startNetworkAndTest() into something like networkDepenciesReady(), because its current name is actually mis-leading - it never starts the test itself.
(Assignee)

Comment 50

2 years ago
https://reviewboard.mozilla.org/r/39525/#review36875

> I would prefer if you would simply resolve the promise right away if disabling the http proxy is not needed.
> That way you can skip the SpecialPowers.pushPrefEnv() call and just run with the default value.

We still need to set it back to false after we have run a test that set it to true; this is set on a test-by-test basis.

> Wouldn't it make sense to roll setupIceServerConfig() into the startNetworkAndTest()?
> I would then rename startNetworkAndTest() into something like networkDepenciesReady(), because its current name is actually mis-leading - it never starts the test itself.

I think I'd prefer to have it be separate to keep the function size down.
(Assignee)

Comment 51

2 years ago
Created attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review commit: https://reviewboard.mozilla.org/r/40453/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40453/
(Assignee)

Comment 52

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/2-3/
Attachment #8729628 - Attachment description: MozReview Request: Bug 1231981 - Part 2: (WIP) Set up TURN server for webrtc mochitests, when necessary. → MozReview Request: Bug 1231981 - Part 3: (WIP) Set up TURN server for webrtc mochitests, when configured to.
(Assignee)

Comment 53

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/1-2/
Attachment #8730920 - Attachment description: MozReview Request: Bug 1231981 - Part 3: Pull in the necessary python dependencies in CI. → MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI.
(Assignee)

Updated

2 years ago
Blocks: 1231975
(Assignee)

Comment 54

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/2-3/
(Assignee)

Comment 55

2 years ago
Ok, I have this working locally with the mochitests in bug 1231975. In CI, it is successfully installing twisted, but as expected the other deps fail to install. I think this is as far as I can go until bug 1256676 is resolved.
(Assignee)

Updated

2 years ago
Attachment #8723196 - Attachment description: MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno → MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r?ahal
Attachment #8723196 - Flags: review?(ahalberstadt)
(Assignee)

Comment 56

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/8-9/
(Assignee)

Comment 57

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/1-2/
Attachment #8731229 - Attachment description: MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. → MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r?ahal
Attachment #8731229 - Flags: review?(ahalberstadt)
(Assignee)

Comment 58

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/3-4/
Attachment #8729628 - Attachment description: MozReview Request: Bug 1231981 - Part 3: (WIP) Set up TURN server for webrtc mochitests, when configured to. → MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r?drno
Attachment #8729628 - Flags: review?(drno)
(Assignee)

Comment 59

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/3-4/
Attachment #8730920 - Attachment description: MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. → MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r?ahal
Attachment #8730920 - Flags: review?(ahalberstadt)
(Assignee)

Comment 60

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/4-5/
(Assignee)

Comment 61

2 years ago
Created attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review commit: https://reviewboard.mozilla.org/r/40615/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40615/
(Assignee)

Comment 62

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

It seems that there's a ton of options here, from doing nothing and just letting the imports fail, to detecting and installing stuff that is missing. Additionally, there's the question of whether we require the webrtc python dependencies to be installed for all mochitest runs, or just ones that contain some webrtc tests.

As a straw-man proposal, I've opted to print some easily visible diagnostics and fail when we detect some modules are missing, and require the webrtc dependencies on all runs.
Attachment #8731462 - Flags: feedback?(ahalberstadt)
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://reviewboard.mozilla.org/r/36405/#review37285

I don't understand what is going on in this file so mostly reviewed for style. I don't like holding things up just for style nits, so use your best judgement on whether to drop or fix. Each comment has multiple infractions, but I only mentioned them once.

::: testing/tools/iceServer/iceServer.py:1
(Diff revision 9)
> +# vim: set ts=4 et sw=4 tw=80

Missing license block

::: testing/tools/iceServer/iceServer.py:52
(Diff revision 9)
> +XOR_MAPPED_ADDRESS = 0x0020
> +SOFTWARE = 0x8022
> +ALTERNATE_SERVER = 0x8023
> +FINGERPRINT = 0x8028
> +
> +def unpackUInt(bytesBuf):

nit: prefer underscores over camel case for function/variable names

::: testing/tools/iceServer/iceServer.py:92
(Diff revision 9)
> +    dest = dest << numBits
> +    mask = (1 << numBits) - 1
> +    dest += source & mask
> +    return dest
> +
> +class StunAttribute:

Classes should inherit from object in python 2, e.g `class StunAttribute(object):`

::: testing/tools/iceServer/iceServer.py:103
(Diff revision 9)
> +#     |                         Value (variable)                ....
> +#     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> +    __attrHeaderFmt = [2,2]
> +    __attrHeaderSize = reduce(operator.add, __attrHeaderFmt)
> +
> +    def __init__(self, attrType = 0, buf = bytearray()):

no spaces around '=' in function signatures

::: testing/tools/iceServer/iceServer.py:135
(Diff revision 9)
> +                raise ValueError("Non-zero padding")
> +            length += 1
> +
> +        return length
> +
> +class StunMessage:

Would be nice to include some docstrings on the classes at least

::: testing/tools/iceServer/iceServer.py:360
(Diff revision 9)
> +    def close(self):
> +        self.port.stopListening()
> +        self.port = None
> +
> +class StunHandler:

nit: two spaces between classes

::: testing/tools/iceServer/iceServer.py:524
(Diff revision 9)
> +            print ("Dropping send indication; no allocation for tuple {}"
> +                    .format(self.getAllocationTuple()))
> +            return
> +
> +        peerAddress = indication.getXorAddress(XOR_PEER_ADDRESS)
> +        if not peerAddress:
> +            print "Dropping send indication, missing XOR-PEER-ADDRESS"

Use print("foobar") consistently, also no space between print and (.
Attachment #8723196 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

https://reviewboard.mozilla.org/r/40453/#review37291

::: testing/mochitest/moz.build:157
(Diff revision 2)
> +TEST_HARNESS_FILES.testing.mochitest.iceServer += [
> +    '/testing/tools/iceServer/iceServer.py',
> +]
> +
> +TEST_HARNESS_FILES.testing.mochitest.websocketToProcessBridge += [
> +    '/testing/tools/websocketToProcessBridge/websocketToProcessBridge.py',
> +]

These camelcased directory names are inconsistent with everything else under /testing.. maybe call it 'iceserver' and 'websocketbridge' ? Or use dashes if you must.

::: testing/mochitest/runtests.py:810
(Diff revision 2)
> +        """Create a websocket server that can launch various processes that"""
> +        """JS needs (eg; ICE server for webrtc testing)"""

These should be a single docstring:
"""Create a websocket server that can launch
various processes that...
"""

::: testing/mochitest/runtests.py:831
(Diff revision 2)
> +            self.log.info("TEST-UNEXPECTED-FAIL | runtests.py | Timed out while"
> +                          " waiting for websocket/process bridge startup.")

self.log.error("runtests.py | Timed out while...")

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:1
(Diff revision 2)
> +from twisted.internet import protocol, reactor
> +import txws

Missing license block.

nit: keep 3rd party imports separate from stdlib imports down below.

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:12
(Diff revision 2)
> +# Handles the spawned process (I/O, process termination)
> +class ProcessSide(protocol.ProcessProtocol):

Might as well make this a docstring, same for SocketSide below.

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:17
(Diff revision 2)
> +# Handles the spawned process (I/O, process termination)
> +class ProcessSide(protocol.ProcessProtocol):
> +    def __init__(self, socketSide):
> +        self.socketSide = socketSide
> +
> +    def outReceived(self, data):

nit: use underscores over camel case

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:43
(Diff revision 2)
> +            try:
> +                self.processSide = ProcessSide(self)
> +                reactor.spawnProcess(self.processSide,
> +                                     commands[data][0],
> +                                     commands[data],
> +                                     usePTY=True)
> +            except:
> +                self.shutdown()

This looks like it could potentially result in silent failures. Can we at least reraise after self.shutdown() so the main process has a chance of pulling the traceback out of the output? Not sure what twisted does with exceptions.

::: testing/tools/websocketToProcessBridge/websocketToProcessBridge.py:58
(Diff revision 2)
> +        if self.processSide:
> +            self.processSide.shutdown()
> +
> +    def shutdown(self):
> +        self.transport.loseConnection()
> +        self.processSide = None

I assume "connectionLost" will get called before we get to this line?
Attachment #8731229 - Flags: review?(ahalberstadt)
Attachment #8730920 - Flags: review?(ahalberstadt)
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

https://reviewboard.mozilla.org/r/40229/#review37293

::: testing/config/marionette_requirements.txt:1
(Diff revision 5)
> --r mozbase_requirements.txt
> +-r mochitest_requirements.txt

Let's create a single new requirements file called "iceserver_requirements.txt" in testing/config. And in it can contain everything, e.g:

ipaddr
passlib
twisted
twxs

Then we can add a second `-r iceserver_requirements.txt` to the main marionette_requirements.txt. We can separate these deps out in the future if we need to.

::: testing/mozharness/scripts/android_emulator_unittest.py:156
(Diff revision 5)
> +        requirements = os.path.join(dirs['abs_test_install_dir'],
> +                                    'config',
> +                                    'mochitest_requirements.txt')

Taking my above suggestion, requirements can be a list of files here. So you can append both "mozbase_requirements.txt" and "iceserver_requirements.txt" in it. No need for one to include the other. In fact, if you didn't want to include iceserver_requirements.txt in marionette_requirements.txt, you could do something similar in desktop_unittest.py.
https://reviewboard.mozilla.org/r/40453/#review37299

::: testing/mochitest/runtests.py:853
(Diff revision 2)
>          if not options.httpdPath:
>              options.httpdPath = os.path.join(options.utilityPath, "components")
>  
>          self.startWebServer(options)
>          self.startWebSocketServer(options, debuggerInfo)
> +        self.startWebsocketToProcessBridge(options)

The plan was still to make a new subsuite right? If so this should be behind an `if options.subsuite == "webrtc"`
https://reviewboard.mozilla.org/r/40615/#review37301

Better than checking for the required packages, is to get the mach command to install these automatically. Do a dxr for `install_pip_package` for an example how.
(Assignee)

Comment 68

2 years ago
https://reviewboard.mozilla.org/r/40453/#review37291

> This looks like it could potentially result in silent failures. Can we at least reraise after self.shutdown() so the main process has a chance of pulling the traceback out of the output? Not sure what twisted does with exceptions.

The intent here is for the websocket client to notice that something has gone wrong, because the connection has closed prematurely, but not catching the exception is probably better.

> I assume "connectionLost" will get called before we get to this line?

No. This is called when the processSide has either died, or was never successfully established, and doesn't need to be shut down. (This is reciprocal; the first of the two to go away calls shutdown on the other)
(Assignee)

Comment 69

2 years ago
https://reviewboard.mozilla.org/r/40453/#review37299

> The plan was still to make a new subsuite right? If so this should be behind an `if options.subsuite == "webrtc"`

Perhaps eventually, but not now. Since the websocket/process bridge is a more generic tool, it probably doesn't make sense to hide it behind such a check even if we did have the subsuite.
(Assignee)

Comment 71

2 years ago
https://reviewboard.mozilla.org/r/40453/#review37291

> nit: use underscores over camel case

All of these functions are inherited. I can change the variables though.
(Assignee)

Comment 72

2 years ago
https://reviewboard.mozilla.org/r/40453/#review37291

> No. This is called when the processSide has either died, or was never successfully established, and doesn't need to be shut down. (This is reciprocal; the first of the two to go away calls shutdown on the other)

I will rename these to be more clear.
(In reply to Byron Campen [:bwc] from comment #71)
> https://reviewboard.mozilla.org/r/40453/#review37291
> 
> > nit: use underscores over camel case
> 
> All of these functions are inherited. I can change the variables though.

It's ok then, as long as the functions are camelCase might as well keep the variables consistent. Feel free to drop that issue.
(Assignee)

Comment 74

2 years ago
https://reviewboard.mozilla.org/r/40229/#review37293

> Let's create a single new requirements file called "iceserver_requirements.txt" in testing/config. And in it can contain everything, e.g:
> 
> ipaddr
> passlib
> twisted
> twxs
> 
> Then we can add a second `-r iceserver_requirements.txt` to the main marionette_requirements.txt. We can separate these deps out in the future if we need to.

I was trying to keep the iceserver requirements (webrtc-specific) separate from the websocketprocessbridge requirements (more generic), but I guess it could be argued that websocketprocessbridge requires the iceserver stuff because iceserver is on the "menu". So, I can just put everything in a requirements file for websocketprocessbridge.
(Assignee)

Comment 75

2 years ago
https://reviewboard.mozilla.org/r/40229/#review37293

> Taking my above suggestion, requirements can be a list of files here. So you can append both "mozbase_requirements.txt" and "iceserver_requirements.txt" in it. No need for one to include the other. In fact, if you didn't want to include iceserver_requirements.txt in marionette_requirements.txt, you could do something similar in desktop_unittest.py.

I think it makes sense for there to be a new requirements file for mochitest (but without marionette's dependencies), since it is confusing for MochitestDesktop to pull in a marionette_requirements.txt, and since mochitest now has additional requirements on top of mozbase that need to be known in multiple places. In fact, I'm still not clear what's required by mochitest but not marionette.
I wouldn't over-architect this dependency thing, I don't think it needs to be perfect (and fwiw, mochitest does depend on marionette). I'm a fan of keeping it simple and when/if things get more complicated, dealing with it then. What I really want is to be able to define a list of requirements files in configuration on a per-suite basis. This is what I filed bug 1256975 for. So just be aware that whatever approach you take will likely be refactored anyway when that gets tackled.
(Assignee)

Updated

2 years ago
Attachment #8723196 - Attachment description: MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r?ahal → MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal
(Assignee)

Comment 77

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/9-10/
(Assignee)

Comment 78

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/2-3/
Attachment #8731229 - Flags: review?(ahalberstadt)
(Assignee)

Comment 79

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/4-5/
(Assignee)

Comment 80

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/5-6/
Attachment #8730920 - Flags: review?(ahalberstadt)
(Assignee)

Comment 81

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/1-2/
Attachment #8731462 - Flags: feedback?(ahalberstadt)
(Assignee)

Comment 82

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/2-3/
Attachment #8730920 - Flags: review?(ahalberstadt)
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

https://reviewboard.mozilla.org/r/40229/#review37705

::: testing/mozharness/scripts/android_emulator_unittest.py:156
(Diff revision 6)
> +        requirements = os.path.join(dirs['abs_test_install_dir'],
> +                                    'config',
> +                                    'mochitest_requirements.txt')

I'm pretty sure this will break most test suites. The desktop_unittest.py script is used by all kinds of suites not restricted to mochitest. Besides, mochitest actually does depend on marionette.

I'd do something like:

    reqs = [
        'marionette_requirements.txt',
        'websocketprocessbridge_requirements.txt',
    ]
    register_virtualenv_module(requirements=reqs)

::: testing/mozharness/scripts/androidx86_emulator_unittest.py:163
(Diff revision 6)
> +        requirements = os.path.join(dirs['abs_test_install_dir'],
> +                                    'config',
> +                                    'mochitest_requirements.txt')

For android, mochitest does not depend on marionette, but this will likely change soon as we'll need to use it to get around addon signing.
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

https://reviewboard.mozilla.org/r/40453/#review37707

Thanks!

::: testing/mochitest/runtests.py:832
(Diff revisions 2 - 3)
>                  sock.close()
>                  break
>              except:
>                  time.sleep(0.1)
>          else:
> -            self.log.info("TEST-UNEXPECTED-FAIL | runtests.py | Timed out while"
> +            self.log.error("TEST-UNEXPECTED-FAIL | runtests.py | Timed out while"

The TEST-UNEXPECTED-FAIL is redundant here. log.error will dump ERROR instead, and both mozharness and treeherder log parsers are set up to detect it.
Attachment #8731229 - Flags: review?(ahalberstadt) → review+

Updated

2 years ago
Attachment #8729628 - Flags: review?(drno) → review+
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

https://reviewboard.mozilla.org/r/39525/#review38207

::: dom/media/tests/mochitest/pc.js:1821
(Diff revision 5)
> +  });
> +
> +  var spawnIceServer = () => new Promise( (resolve, reject) => {
> +    iceServerWebsocket = new WebSocket("ws://localhost:8191/");
> +    iceServerWebsocket.onopen = (event) => {
> +      info("launcher websocket open, running ICE Server...");

running = starting ?

::: dom/media/tests/mochitest/pc.js:1829
(Diff revision 5)
> +
> +    iceServerWebsocket.onmessage = event => {
> +      // The first message will contain the iceServers configuration, subsequent
> +      // messages are just logging.
> +      info("ICE Server: " + event.data);
> +      resolve(event.data);

I think it does not break anything right now, but I guess that if you keep resolving the Promise with new/different data if something later calls the same promise he would not get the configuration but the logging data instead?!

::: dom/media/tests/mochitest/pc.js:1842
(Diff revision 5)
> +  if (!useIceServer) {
> +    info("Skipping ICE Server for this test");
> +    iceServersArray = [];
> +    return enableHttpProxy(true);
> +  }

I think |iceServersArray| should simply be default inited to []. Then you can remove that assignment.

And the enabledHttpProxy() call is just extra over head, as it being enabled is still the default, or?

Once you remove these two line I think we should move the remaining check to the top of setupIceServerConfig() to exit as early as possible.

::: dom/media/tests/mochitest/pc.js:1850
(Diff revision 5)
> +    return enableHttpProxy(true);
> +  }
> +
> +  return enableHttpProxy(false)
> +    .then(spawnIceServer)
> +    .then(iceServersStr => { iceServersArray = JSON.parse(iceServersStr); });

You should guard this with a catch(), especially the JSON parsing.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:502
(Diff revision 5)
>      if (!cb_arg)
>        return;
>      ctx = cand->ctx;
>  
>      ctx->uninitialized_candidates--;
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE(%s)/CAND(%s): initialized",ctx->label,cand->codeword);

How about including the |unitialized_candidates| in the log message?
(Assignee)

Comment 86

2 years ago
https://reviewboard.mozilla.org/r/39525/#review38207

> I think it does not break anything right now, but I guess that if you keep resolving the Promise with new/different data if something later calls the same promise he would not get the configuration but the logging data instead?!

My understanding is that promises are resolved at most once, and any subsequent calls to resolve are ignored. A subsequent waiter will get the original argument passed to resolve.

> I think |iceServersArray| should simply be default inited to []. Then you can remove that assignment.
> 
> And the enabledHttpProxy() call is just extra over head, as it being enabled is still the default, or?
> 
> Once you remove these two line I think we should move the remaining check to the top of setupIceServerConfig() to exit as early as possible.

We need to re-enable http proxy support in case the previous test disabled it.

> You should guard this with a catch(), especially the JSON parsing.

I can look into this, although errors here do percolate up.

> How about including the |unitialized_candidates| in the log message?

Sure, why not.
(Assignee)

Comment 87

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/3-4/
Attachment #8731229 - Attachment description: MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r?ahal → MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal
(Assignee)

Comment 88

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/5-6/
Attachment #8729628 - Attachment description: MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r?drno → MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno
(Assignee)

Comment 89

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/6-7/
Attachment #8730920 - Flags: review?(ahalberstadt)
(Assignee)

Comment 90

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/3-4/
Attachment #8731462 - Attachment description: MozReview Request: Bug 1231981 - Part 5: Ask for python packages we need if not present. → MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r?ahal
Attachment #8731462 - Flags: review?(ahalberstadt)
(Assignee)

Comment 91

2 years ago
https://reviewboard.mozilla.org/r/39525/#review38207

> We need to re-enable http proxy support in case the previous test disabled it.

I've changed how iceServersArray is initted, the rest I can't do.
(Assignee)

Comment 92

2 years ago
https://reviewboard.mozilla.org/r/39525/#review38207

> I can look into this, although errors here do percolate up.

Errors in there end up getting caught here:

https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/head.js#325

This does not call SimpleTest.finish() though, so the test hangs. That's probably easily fixable.
(Assignee)

Comment 93

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/6-7/
(Assignee)

Comment 94

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/7-8/
(Assignee)

Comment 95

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/4-5/
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

https://reviewboard.mozilla.org/r/40229/#review38731

Thanks, looks good! (I assume that failing try run was from an earlier version of this patch?)
Attachment #8730920 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 97

2 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #96)
> Comment on attachment 8730920 [details]
> MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python
> dependencies in CI. r?ahal
> 
> https://reviewboard.mozilla.org/r/40229/#review38731
> 
> Thanks, looks good! (I assume that failing try run was from an earlier
> version of this patch?)

Try is failing, but it failing in the way I expect given that the deps aren't there yet.
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

https://reviewboard.mozilla.org/r/40615/#review38741

::: python/mozbuild/mozbuild/virtualenv.py:449
(Diff revision 5)
> +    def install_pip_requirements_file(self, requirements_filename):
> +        """Installs packages in |requirements_filename| via pip.
> + 
> +           Any requirements that are already present are ignored.
> +        """
> +
> +        import pkg_resources
> +        with open(requirements_filename, 'r') as f:
> +            requirements = f.readlines()
> +
> +        # This will eat comments and such
> +        for req in pkg_resources.parse_requirements(requirements):
> +            self.install_pip_package(str(req))

I think this function needs a build peer review which I am not. Unlike for /testing, they are a bit more strict on the module system. I'd run it by gps.

::: python/mozbuild/mozbuild/virtualenv.py:460
(Diff revision 5)
> +        import pkg_resources
> +        with open(requirements_filename, 'r') as f:
> +            requirements = f.readlines()
> +
> +        # This will eat comments and such
> +        for req in pkg_resources.parse_requirements(requirements):

I'm not sure if it makes a difference, but you might want to use pip.req.parse_requirements() here instead.

::: testing/mochitest/runtests.py:817
(Diff revision 5)
>          """
>  
>          command = [sys.executable,
>                     os.path.join("websocketprocessbridge",
>                                  "websocketprocessbridge.py")]
> +        os.environ['PYTHONPATH'] = ":".join(p for p in sys.path)

os.pathsep.join(...)

This seems reasonable to me. Consider this an f+.
Attachment #8731462 - Flags: review?(ahalberstadt)
(Assignee)

Comment 99

2 years ago
https://reviewboard.mozilla.org/r/40615/#review38741

> I'm not sure if it makes a difference, but you might want to use pip.req.parse_requirements() here instead.

I tried that, I could find no programmatic way to get back to the string form with the stuff yielded by pip.req.parse_requirements() (__str__ has a bunch of stuff in it that the pip command line doesn't like)
(Assignee)

Comment 100

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/10-11/
(Assignee)

Comment 101

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/4-5/
(Assignee)

Comment 102

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/7-8/
(Assignee)

Comment 103

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/8-9/
(Assignee)

Comment 104

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/5-6/
Attachment #8731462 - Attachment description: MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r?ahal → MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r?gps
Attachment #8731462 - Flags: review?(gps)

Updated

2 years ago
Attachment #8731462 - Flags: review?(gps)
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

https://reviewboard.mozilla.org/r/40615/#review38949

::: python/mozbuild/mozbuild/virtualenv.py:449
(Diff revision 6)
> +    def install_pip_requirements_file(self, requirements_filename):
> +        """Installs packages in |requirements_filename| via pip.
> +
> +           Any requirements that are already present are ignored.
> +        """

This should be invoking `pip install -r <path>` because processing a requirements file is so much more than extracting packages from individual lines. For example, requirements files can specify additional servers advertising custom packages.
https://reviewboard.mozilla.org/r/40229/#review38951

::: testing/tools/websocketprocessbridge/websocketprocessbridge_requirements.txt:1
(Diff revision 9)
> +twisted

Please pin versions and add `--hash` to packages in requirements files. This ensures:

a) Versions don't change out from under us (this helps with deterministic operations and prevents breakage when 3rd party packages are upgraded)

b) Content is verified against tampering (malicious or not). This helps keep the Firefox release process secure.

https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/ansible/roles/hg-web/files/requirements-replication.txt is an example.
(Assignee)

Comment 107

2 years ago
https://reviewboard.mozilla.org/r/40229/#review38951

> Please pin versions and add `--hash` to packages in requirements files. This ensures:
> 
> a) Versions don't change out from under us (this helps with deterministic operations and prevents breakage when 3rd party packages are upgraded)
> 
> b) Content is verified against tampering (malicious or not). This helps keep the Firefox release process secure.
> 
> https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/ansible/roles/hg-web/files/requirements-replication.txt is an example.

I'll do this once bug 1256676 is done.
(Assignee)

Comment 108

2 years ago
https://reviewboard.mozilla.org/r/40615/#review38949

> This should be invoking `pip install -r <path>` because processing a requirements file is so much more than extracting packages from individual lines. For example, requirements files can specify additional servers advertising custom packages.

Easy enough.
(Assignee)

Comment 109

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/11-12/
(Assignee)

Comment 110

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/5-6/
(Assignee)

Comment 111

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/8-9/
(Assignee)

Comment 112

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/9-10/
(Assignee)

Comment 113

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/6-7/
Attachment #8731462 - Flags: review?(gps)
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

https://reviewboard.mozilla.org/r/40615/#review38971

::: python/mozbuild/mozbuild/virtualenv.py:454
(Diff revision 7)
> +    def install_pip_requirements_file(self, requirements_filename):
> +        """Installs packages in |requirements_filename| via pip."""
> +
> +        args = [
> +            'install',
> +            '--use-wheel',

I'm 99% sure that --use-wheel is the default in modern versions of pip. And as of yesterday, we have the latest version of pip vendored in mozilla-central. So this can be dropped.
Attachment #8731462 - Flags: review?(gps) → review+
(Assignee)

Comment 115

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/7-8/
Attachment #8731462 - Attachment description: MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r?gps → MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps
(Assignee)

Comment 116

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/12-13/
Attachment #8730920 - Attachment description: MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r?ahal → MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal
(Assignee)

Comment 117

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/6-7/
(Assignee)

Comment 118

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/9-10/
(Assignee)

Comment 119

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/10-11/
(Assignee)

Comment 120

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/8-9/
(Assignee)

Comment 121

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/11-12/
(Assignee)

Comment 122

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/9-10/
(Assignee)

Comment 123

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/12-13/
(Assignee)

Comment 124

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/10-11/
(Assignee)

Comment 125

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/13-14/
(Assignee)

Comment 126

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/11-12/
(Assignee)

Comment 127

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/14-15/
(Assignee)

Comment 128

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/12-13/
(Assignee)

Comment 129

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/13-14/
(Assignee)

Comment 130

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/7-8/
(Assignee)

Comment 131

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/10-11/
(Assignee)

Comment 132

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/15-16/
(Assignee)

Comment 133

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/13-14/
(Assignee)

Comment 134

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/8-9/
(Assignee)

Comment 135

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/11-12/
(Assignee)

Comment 136

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/16-17/
(Assignee)

Comment 137

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/14-15/
(Assignee)

Comment 138

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/9-10/
(Assignee)

Comment 139

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/12-13/
(Assignee)

Comment 140

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/17-18/
(Assignee)

Comment 141

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/15-16/
(Assignee)

Comment 142

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/18-19/
(Assignee)

Comment 143

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/16-17/
(Assignee)

Comment 144

2 years ago
https://reviewboard.mozilla.org/r/40229/#review38951

> I'll do this once bug 1256676 is done.

I've set the version numbers, but this does not seem to work with the version of pip the testers use.
(Assignee)

Comment 145

2 years ago
https://reviewboard.mozilla.org/r/40229/#review38951

> I've set the version numbers, but this does not seem to work with the version of pip the testers use.

(I should clarify; the version numbers work fine, but the version of pip in use doesn't grok --hash)
(Assignee)

Comment 146

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/19-20/
(Assignee)

Comment 147

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/17-18/
(Assignee)

Comment 148

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/20-21/
(Assignee)

Comment 149

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/18-19/
(Assignee)

Comment 150

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/21-22/
(Assignee)

Comment 151

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/19-20/
(Assignee)

Comment 152

2 years ago
Created attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review commit: https://reviewboard.mozilla.org/r/46415/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46415/
Attachment #8741352 - Flags: review?(gps)
(Assignee)

Comment 153

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/14-15/
(Assignee)

Comment 154

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/10-11/
(Assignee)

Comment 155

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/13-14/
(Assignee)

Comment 156

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/22-23/
(Assignee)

Comment 157

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/20-21/
(Assignee)

Comment 158

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/15-16/
(Assignee)

Comment 159

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/11-12/
(Assignee)

Comment 160

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/14-15/
(Assignee)

Comment 161

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/23-24/
(Assignee)

Comment 162

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/21-22/
(Assignee)

Comment 163

2 years ago
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/1-2/
https://reviewboard.mozilla.org/r/40615/#review43473

::: testing/mochitest/runtests.py:818
(Diff revision 22)
>          """
>  
>          command = [sys.executable,
>                     os.path.join("websocketprocessbridge",
>                                  "websocketprocessbridge.py")]
> +        os.environ['PYTHONPATH'] = os.pathsep.join(p for p in sys.path)

This feels like a hack. Either the virtualenv has its paths configured correctly or we supplement PYTHONPATH with just the paths not in the virtualenv.
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

https://reviewboard.mozilla.org/r/46415/#review43475
Attachment #8741352 - Flags: review?(gps) → review+
(Assignee)

Comment 166

2 years ago
https://reviewboard.mozilla.org/r/40615/#review43473

> This feels like a hack. Either the virtualenv has its paths configured correctly or we supplement PYTHONPATH with just the paths not in the virtualenv.

It is a hack. Right now the virtualenv doesn't have the system python path, while the functions that install packages into the virtualenv do see the python path. This means that if you have twisted in your system python, it will not be installed in the virtualenv, and then the virtualenv python won't find it.
(Assignee)

Comment 167

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/16-17/
(Assignee)

Comment 168

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/12-13/
(Assignee)

Comment 169

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/15-16/
(Assignee)

Comment 170

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/24-25/
(Assignee)

Comment 171

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/22-23/
(Assignee)

Comment 172

2 years ago
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/2-3/
(Assignee)

Comment 173

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/13-14/
(Assignee)

Comment 174

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/16-17/
(Assignee)

Comment 175

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/25-26/
(Assignee)

Comment 176

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/23-24/
(Assignee)

Comment 177

2 years ago
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/3-4/
(Assignee)

Comment 178

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/17-18/
(Assignee)

Comment 179

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/14-15/
(Assignee)

Comment 180

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/17-18/
(Assignee)

Comment 181

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/26-27/
(Assignee)

Comment 182

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/24-25/
(Assignee)

Comment 183

2 years ago
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/4-5/
(Assignee)

Comment 184

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a244bcd9911b7fe2da9387e07acb3ca5f7e1d0e3
Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

https://hg.mozilla.org/integration/mozilla-inbound/rev/de3f062c9a999718143e4567c475a260f75a6f2e
Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fcd0e98cc34ee7e24d22c08e4469f2cbc7d548
Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

https://hg.mozilla.org/integration/mozilla-inbound/rev/507145be7f820701cc3980050200141f7afc9457
Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a0d061f75488cd6a44e59d9858742ae61b48f04
Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

https://hg.mozilla.org/integration/mozilla-inbound/rev/15472b261c9b0b7dc15716d4d5f5c02a606afef9
Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps
backed out for mass mochitest bustage like : https://treeherder.mozilla.org/logviewer.html#?job_id=26551894&repo=mozilla-inbound

 14:21:56     INFO -  MochitestServer : launching [u'/home/worker/workspace/build/tests/bin/xpcshell', '-g', '/home/worker/workspace/build/application/firefox', '-v', '170', '-f', '/home/worker/workspace/build/tests/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmpQNmhY_.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/home/worker/workspace/build/tests/mochitest/server.js']
 14:21:56     INFO -  runtests.py | Server pid: 1198
 14:21:56     INFO -  runtests.py | Websocket server pid: 1201
 14:21:56     INFO -  runtests.py | websocket/process bridge pid: 1206
 14:22:06     INFO -  0 ERROR runtests.py | Timed out while waiting for websocket/process bridge startup.
 14:22:06     INFO -  Stopping web server
 14:22:06     INFO -  Stopping web socket server
 14:22:06     INFO -  Stopping websocket/process bridge
 14:22:06     INFO -  websocket/process bridge listening on port 8191
 14:22:06     INFO -  Stopping web server
 14:22:06     INFO -  Stopping web socket server
14:22:06 INFO - Stopping websocket/process bridge
Flags: needinfo?(docfaraday)
(Assignee)

Comment 187

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/15-16/
(Assignee)

Comment 188

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/18-19/
(Assignee)

Comment 189

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/27-28/
(Assignee)

Comment 190

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/25-26/
(Assignee)

Comment 191

2 years ago
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/5-6/
(Assignee)

Comment 192

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/18-19/
Attachment #8741352 - Attachment description: MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r?gps → MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps
(Assignee)

Comment 193

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/16-17/
(Assignee)

Comment 194

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/19-20/
(Assignee)

Comment 195

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/28-29/
(Assignee)

Comment 196

2 years ago
Comment on attachment 8731462 [details]
MozReview Request: Bug 1231981 - Part 5: Install python packages we need in the virtualenv if not present. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40615/diff/26-27/
(Assignee)

Comment 197

2 years ago
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/6-7/
(Assignee)

Comment 198

2 years ago
Should be fine now.
Flags: needinfo?(docfaraday)

Updated

2 years ago
Target Milestone: mozilla48 → mozilla49

Updated

2 years ago
Depends on: 1268022

Updated

2 years ago
Depends on: 1268032
Depends on: 1268092
Backed out in https://hg.mozilla.org/mozilla-central/rev/86730d0a8209 for breaking local mochitests.
Status: RESOLVED → REOPENED
status-firefox49: fixed → affected
Resolution: FIXED → ---
Target Milestone: mozilla49 → ---
(Assignee)

Comment 202

2 years ago
Created attachment 8746218 [details]
MozReview Request: Bug 1231981 - Part 2.1: Only run the websocket/process bridge for media tests. r=ahal

Review commit: https://reviewboard.mozilla.org/r/49311/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49311/
Attachment #8746218 - Flags: review?(ahalberstadt)
(Assignee)

Comment 203

2 years ago
Comment on attachment 8723196 [details]
MozReview Request: Bug 1231981 - Part 1: Very basic test TURN server for running in CI. r=drno, r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36405/diff/19-20/
(Assignee)

Comment 204

2 years ago
Comment on attachment 8731229 [details]
MozReview Request: Bug 1231981 - Part 2: A websocket-to-process bridge script that can be used by JS to launch an ICE server for testing. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40453/diff/17-18/
(Assignee)

Comment 205

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/20-21/
(Assignee)

Comment 206

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/29-30/
(Assignee)

Comment 207

2 years ago
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/7-8/
(Assignee)

Updated

2 years ago
Attachment #8731462 - Attachment is obsolete: true
Comment on attachment 8746218 [details]
MozReview Request: Bug 1231981 - Part 2.1: Only run the websocket/process bridge for media tests. r=ahal

https://reviewboard.mozilla.org/r/49311/#review46535

Looks good, though the variable name is off. r+ with that fixed.

::: testing/mochitest/runtests.py:826
(Diff revision 1)
>          self.log.info("runtests.py | websocket/process bridge pid: %d"
>                        % self.websocketProcessBridge.pid)
>  
>          # ensure the server is up, wait for at most ten seconds
>          for i in range(1,100):
> +            if self.websocketprocessbridge.proc.poll() is not None:

This needs to be camel cased.
Attachment #8746218 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 209

2 years ago
Comment on attachment 8746218 [details]
MozReview Request: Bug 1231981 - Part 2.1: Only run the websocket/process bridge for media tests. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49311/diff/1-2/
Attachment #8746218 - Attachment description: MozReview Request: Bug 1231981 - Part 2.1: Only run the websocket/process bridge for media tests. r?ahal → MozReview Request: Bug 1231981 - Part 2.1: Only run the websocket/process bridge for media tests. r=ahal
(Assignee)

Comment 210

2 years ago
Comment on attachment 8729628 [details]
MozReview Request: Bug 1231981 - Part 3: Set up TURN server for webrtc mochitests, when configured to. r=drno

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39525/diff/21-22/
(Assignee)

Comment 211

2 years ago
Comment on attachment 8730920 [details]
MozReview Request: Bug 1231981 - Part 4: Pull in the necessary python dependencies in CI. r=ahal

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40229/diff/30-31/
(Assignee)

Comment 212

2 years ago
Comment on attachment 8741352 [details]
MozReview Request: Bug 1231981 - Part 6: Make sure we have pywin32 in the virtualenv for mochitest. r=gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46415/diff/8-9/

Updated

2 years ago
Duplicate of this bug: 865296

Updated

2 years ago
Depends on: 1279259
You need to log in before you can comment on or make changes to this bug.