686320 - Allow Mozmill to probe the OS for a free port instead of hardcoded value in JSBridge

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, enhancement)

Description

[David Burns :automatedtester]

Allow JSBridge to use a port prober so that we can scale tests. At the moment it uses the same port which means that tests need to be run in serial. We need to start running tests faster.

Comment 1

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

As a workaround for now, you can pass-in the specific port JSBridge has to use to connect to the browser. But yes, leaving this up to JSBridge sounds like the best solution here.

Comment 2

[David Burns :automatedtester]

Attachment: Mozmill Free port probing

Added patch that allows Mozmill to probe the OS for a free port so that we can run more tests in parallel

Comment 3

[Jeff Hammel]

Comment on attachment 560172 [details] [diff] [review]
Mozmill Free port probing

This looks good.  A few suggestions:

- eliminate the global JSBRIDGE_PORT altogether since it no longer carries any information
- use jsbridge_port = None in init and create methods
- change the check to `if not jsbridge_port` vs `if jsbridge_port == 0`

r+ with changes

Comment 4

[David Burns :automatedtester]

Attachment: Mozmill Free port probing v2

I have updated the patch with review comments. Please feel free to push into the repo

Comment 5

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

Comment on attachment 560195 [details] [diff] [review]
Mozmill Free port probing v2

>+    @classmethod
>+    def free_port(self):

Could we use a better name for this class method? For me it's confusing because when reading 'free' I think about freeing an already used port. What about 'find_free_port'?

Comment 6

[David Burns :automatedtester]

We could change it to random_port. Ideally this method would be private. Please remember that this method is only going to be used within the Mozmill class so the method name is inconsequential.

Comment 7

[Jeff Hammel]

There is also the js side of things:  https://github.com/mozautomation/mozmill/blob/master/jsbridge/jsbridge/extension/components/cmdarg.js#L37  

This should probably just go away and the extension not register itself if a port is not given on the command line (or as a preference?)

Comment 8

[Jeff Hammel]

actually, should the free_port method be its own standalone function vs a classmethod (regardless of name)?

Comment 9

[David Burns :automatedtester]

I made it a method as part of the Mozmill object since I didn't see any value in it being a standalone function. honestly don't mind either way, I was just trying to keep things a little cleaner.

If you would like me to change the name/ scope just assign back and I will do the work.

re Comment 7, would you like me to make the changes to the extension as part of this patch?

Comment 10

[Jeff Hammel]

Attachment: port probing, version :jhammel

My modifications to the patch

Comment 11

[Jeff Hammel]

So this spectacularly fails on the mutt tests.  I'll investigate this further (likely next week), but a few notes of things that need to be changed:

- there is a bug that Firefox does not preserve the command line on restart (I can't find it right now, but we should link that information here).  So -jsbridge 12345 (e.g.) is not preserved and instead the fallback 24242 does not work :( While the Firefox bug should be fixed, we need a fix in the interim.  So I guess we should abuse preferences for storage and stick the port there, *not* have a fallback in the JS side of jsbridge.  Of course, this should all be reversed when the upstream bug is fixed :(

- we should subclass MozRunner as MozMillRunner and put the jsbridge port there.  This will ignore the duplication between .create() and .__init__().  Right now, we just use the convention that the runner's port is set to the same as the port passed to these methods :(

Comment 12

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

(In reply to David Burns :automatedtester from comment #6)
> We could change it to random_port. Ideally this method would be private.
> Please remember that this method is only going to be used within the Mozmill
> class so the method name is inconsequential.

Whether of a method is private or public a meaningful name always helps to discover the underlying logic. A method is like an action, and in this case it's unclear for me what it is doing when I see its name. 'free' has a couple of meanings, and random is not a verb. Why not call it find_free_port or simply find_port?

Comment 13

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

Have we made any progress on this bug in the last quarter? Looks like it has been stalled. It's getting important now that we want to run tests in parallel.

Comment 14

[Jeff Hammel]

No, AFAIK no one is actively working on this.  IIRC, there is a big bug with command line arguments that they will be reset to defaults (!) if the browser is reset.  So this needs to be fixed, verified or worked around.  Has anyone tried running a restart test with jsbridge port != 24242?  I think this will break.

Comment 15

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

Seems to be blocked by bug 675787.

Comment 16

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

Bug doesn't block as any longer. The issue reported over there seems to be a regression in Mozmill 2. I will follow-up on this patch and finalize it today with some other improvements to the jsbridge extension's command line handler.

Comment 17

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

Attachment: Patch (hotfix-1.5) [backed-out]

Pointer to Github pull-request

Comment 18

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

Comment on attachment 614380 [details]
Patch (hotfix-1.5) [backed-out]

Ok, that adds the port probe feature to Mozmill and JSBridge. With the method in the MozMill class it wouldn't have been possible to run jsbridge from the command line and let it search for a free port.

Jeff, I kinda would like to get feedback from you on that patch to ensure everything is correct. Please note that this patch is for hotfix-1.5. Once it has been reviewed I will update the code for master and hotfix-2.0.

Comment 19

[Jeff Hammel]

Comment on attachment 614380 [details]
Patch (hotfix-1.5) [backed-out]

This is probably fine.  I am much more interested in the 2.x case

Comment 20

[cmtalbert]

Comment on attachment 614380 [details]
Patch (hotfix-1.5) [backed-out]

This looks good.  Make sure you test it with a restart test and you have my review.

r+

Clint

Comment 21

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

I run manual tests for the following cases:
* jsbridge CLI, mozmill/mozmill-restart CLI
** Auto-detection of the port
** Specifying a specific port

Until now I only tested it on OS X. So I will make sure to also run tests on Linux and Windows before I check-in the code.

For the Mozmill 2.0 case I will make sure to add some Mutt tests.

Comment 22

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

Tests on Windows and Linux were successful. So I merged the code with the hotfix-1.5 branch:

https://github.com/mozautomation/mozmill/compare/9c1112f...bddd612

I will work on the master and hotfix-2.0 fix once bug 675787 has been fixed.

Comment 23

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

With this change we broke our functional testrun. It's not clear yet what's happening but investigation will happen on bug 746147. So if necessary we need an additional fix for Mozmill 1.5.11.

Comment 24

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

Attachment: Patch (hotfix-1.5: Clean Mozrunner args) [checked-in]

Pointer to Github pull-request

Comment 25

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

Comment on attachment 615858 [details]
Patch (hotfix-1.5: Clean Mozrunner args) [checked-in]

Landed on hotfix-1.5 as:
https://github.com/mozautomation/mozmill/commit/c246b703eddcd362786ed4926abdd4dbfca2fe90

Comment 26

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

Note for @self: For the Mozmill 2 patch we have to make sure to also test the startUserShutdown method. It's broken in Mozmill 1.5 and causes the jsbridge connection not to be established.

Comment 27

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

Ok, so we are still dependent on bug 399317, but to circumvent that we should simply set a preference on first start or when the profile gets reset on the Python side. So the jsbridge extension code can pick up this port and can connect successfully. I will do some testing on it.

Comment 28

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

If that works we could even get rid of the jsbridge command line handler at all.

Comment 29

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

That seems to work fine. So I will kill the command line handler too.

Beside that I will also have to check why we create the server three times on the extension side:

$ mozmill -b /Applications/Firefox/Nightly.app/ -t mutt/mutt/tests/js/frame/user_restart_multiple/
INFO | *************** + 58389
INFO | 
INFO | jsbridge: Server started on port 58389
INFO | 
INFO | jsbridge: Server started on port 58389
INFO | 
INFO | jsbridge: Server started on port 58389
INFO |

Comment 30

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

Attachment: Patch

Pointer to Github pull-request

Comment 31

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

Comment on attachment 632954 [details]
Patch

First sorry for the massive patch. I probably should have split it into sub sections. :( But now it's too late. So here the things I have updated/changed:

* Refactored jsbridge code modules to allow an easy import
* Jsbridge debugging via --debug option
* Jsbridge port is not selectable anymore via --port but gets auto-assigned and stored in the extensions.jsbridge.port preference
* Jsbridge does no longer implement a command line handler in the xpcom module. Its not necessary by using the preference.
* Removed overlays and unnecessary files (why those were there?)
* Removed old server.js module because we have dropped support for 1.9.2 and the nspr module will always be available now

Beside all that the mozmill's __init.py__ module is a mess when directly working with with Mozmill base class. So I had to place some workarounds into the module. I will followup on this later to make it clean.

Comment 32

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

Also keep in mind that the code on the 1.5 branch is busted! Reason is that we do not use a preference there and any user initiated restart of the application will cause a disconnect. We have disabled all of our tests which were affected by this for now. We could backout the patch and release a 1.5.13 to be safe but as long as no-one cries I wouldn't bother.

Comment 33

[Jeff Hammel]

> * Removed overlays and unnecessary files (why those were there?)

I've often wondered the same thing o_O

Comment 34

[Jeff Hammel]

> * Removed old server.js module because we have dropped support for 1.9.2 and the nspr module will always be available now

I forget...have we alerted people to this yet?

Comment 35

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

(In reply to Jeff Hammel [:jhammel] from comment #34)
> I forget...have we alerted people to this yet?

Alerted why? Mozmill 2.0 will no longer support 1.9.2 which is already dead. Or are you asking something else?

Comment 36

[Jeff Hammel]

Comment on attachment 632954 [details]
Patch

Outside of a few nits, this looks great.  I'll go ahead and r+ but please fix the nits before landing.  Also, as asked in https://github.com/mozautomation/mozmill/pull/45/files#r986607 , what happens if the preference isn't set?  While I don't think we should have to do anything (i.e. just not starting the server is fine), we probably shouldn't, say, crash firefox or what not

Comment 37

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

Pushed:
https://github.com/mozautomation/mozmill/commit/26c45bcba0587b5145b974d8e7688d3a5a58a4ae

(In reply to Jeff Hammel [:jhammel] from comment #36)
> https://github.com/mozautomation/mozmill/pull/45/files#0 , what happens if
> the preference isn't set?  While I don't think we should have to do anything
> (i.e. just not starting the server is fine), we probably shouldn't, say,
> crash firefox or what not

Replied over there. We are fine here because no server is getting created.

Comment 38

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

So this patch has been caused a regression in Mozmill 1.5.11 where restart tests do not work anymore if an user initiated restart has to be done. Without setting the preference I don't see an easy way how to fix that. I have talked to Jeff on IRC and we are going to backout this patch on hotfix-1.5 for a Mozmill 1.5.13 release:

https://github.com/mozautomation/mozmill/commit/3d68d17759874eae75a5fc13d8c05754e5989180

Comment 39

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

Actually I missed a little piece yesterday. I haven't tested the patch with ESR 10.0.x and as it turned out this branch doesn't have the appShell property in Services:

Error: Services.appShell is undefined
Source File: resource://jsbridge/modules/Sockets.jsm
Line: 117

I will come up with a small follow-up patch to get this fixed.

Comment 40

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

Attachment: Patch (appShell)

Pointer to Github pull-request

Comment 41

[David Burns :automatedtester]

Comment on attachment 633437 [details]
Patch (appShell)

looks good to me

Comment 42

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

Follow-up patch landed:
https://github.com/mozautomation/mozmill/commit/1bd85c36a12a5ce355c70f8c947c16c4ecdba8c3