Closed Bug 686320 Opened 10 years ago Closed 10 years ago

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

Categories

(Testing Graveyard :: Mozmill, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: automatedtester, Assigned: whimboo)

References

Details

(Whiteboard: [mozmill-2.0+][mozmill-1.5.11+][mozmill-1.5-backed-out])

Attachments

(6 files, 1 obsolete file)

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.
Assignee: nobody → dburns
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.
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Mozmill Free port probing (obsolete) — Splinter Review
Added patch that allows Mozmill to probe the OS for a free port so that we can run more tests in parallel
Attachment #560172 - Flags: review?(jhammel)
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
Attachment #560172 - Flags: review?(jhammel) → review+
I have updated the patch with review comments. Please feel free to push into the repo
Attachment #560172 - Attachment is obsolete: true
Assignee: dburns → jhammel
Assignee: jhammel → dburns
Whiteboard: [mozmill-1.5.5?][mozmill-2.0?]
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'?
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.
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?)
actually, should the free_port method be its own standalone function vs a classmethod (regardless of name)?
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?
My modifications to the patch
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 :(
(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?
Assignee: dburns → nobody
Component: Mozmill Utilities → Mozmill
QA Contact: mozmill-utilities → mozmill
Whiteboard: [mozmill-1.5.5?][mozmill-2.0?] → [mozmill-1.5.6?][mozmill-2.0?]
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.
Whiteboard: [mozmill-1.5.6?][mozmill-2.0?] → [mozmill-1.5.9?][mozmill-2.0?]
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.
Whiteboard: [mozmill-1.5.9?][mozmill-2.0?] → [mozmill-1.5.10?][mozmill-2.0?]
Assignee: nobody → hskupin
Whiteboard: [mozmill-1.5.10?][mozmill-2.0?] → [mozmill-1.5.10+][mozmill-2.0+]
Seems to be blocked by bug 675787.
Depends on: 675787
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.
No longer depends on: 675787
Pointer to Github pull-request
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.
Attachment #614380 - Flags: review?(ctalbert)
Attachment #614380 - Flags: feedback?(jhammel)
Attachment #614380 - Attachment description: Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/19 → Patch (hotfix-1.5)
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
Attachment #614380 - Flags: feedback?(jhammel) → feedback+
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
Attachment #614380 - Flags: review?(ctalbert) → review+
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.
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.
Whiteboard: [mozmill-1.5.10+][mozmill-2.0+] → [mozmill-2.0+][mozmill-1.5.10+][mozmill-1.5.10-fixed]
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.
Depends on: 746147
Whiteboard: [mozmill-2.0+][mozmill-1.5.10+][mozmill-1.5.10-fixed] → [mozmill-2.0+][mozmill-1.5.10+][mozmill-1.5.11+]
Attachment #615858 - Attachment description: Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/24 → Patch (Followup for clean Mozrunner args)
Attachment #615858 - Flags: review?(ctalbert)
Attachment #615858 - Flags: review?(ctalbert) → review+
Attachment #614380 - Attachment description: Patch (hotfix-1.5) → Patch (hotfix-1.5) [checked-in]
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
Attachment #615858 - Attachment description: Patch (Followup for clean Mozrunner args) → Patch (hotfix-1.5: Clean Mozrunner args) [checked-in]
Whiteboard: [mozmill-2.0+][mozmill-1.5.10+][mozmill-1.5.11+] → [mozmill-2.0+][mozmill-1.5.11+][mozmill-1.5.11-fixed]
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.
Blocks: 747299
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.
Depends on: 399317
If that works we could even get rid of the jsbridge command line handler at all.
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 |
Attached file Patch
Pointer to Github pull-request
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.
Attachment #632954 - Attachment description: Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/45 → Patch
Attachment #632954 - Flags: review?(jhammel)
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.
> * Removed overlays and unnecessary files (why those were there?)

I've often wondered the same thing o_O
> * 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?
(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 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
Attachment #632954 - Flags: review?(jhammel) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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
Whiteboard: [mozmill-2.0+][mozmill-1.5.11+][mozmill-1.5.11-fixed] → [mozmill-2.0+][mozmill-1.5.11+][mozmill-1.5-backed-out]
Attachment #614380 - Attachment description: Patch (hotfix-1.5) [checked-in] → Patch (hotfix-1.5) [backed-out]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file Patch (appShell)
Pointer to Github pull-request
Attachment #633437 - Attachment description: Pointer to Github pull request: https://github.com/mozautomation/mozmill/pull/49 → Patch (appShell)
Attachment #633437 - Flags: review?(jhammel)
Comment on attachment 633437 [details]
Patch (appShell)

looks good to me
Attachment #633437 - Flags: review?(jhammel) → review+
Follow-up patch landed:
https://github.com/mozautomation/mozmill/commit/1bd85c36a12a5ce355c70f8c947c16c4ecdba8c3
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
QA Contact: hskupin
You need to log in before you can comment on or make changes to this bug.