Closed
Bug 967595
Opened 11 years ago
Closed 11 years ago
[mozrunner] Calling process_handler.proc.poll() returns 0 if the application is still running after a restart
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
whimboo
:
review+
andrei
:
feedback+
|
Details | Diff | Splinter Review |
So on bug 966234 we have experienced some very strange process disconnect errors with Mozmill and Firefox. That only happens when the application has been restarted and Mozmill wants to shutdown Firefox afterward. In such a case the self.returncode returns 0 while it should return None. As result we return too early from mozrunner.wait() and run into major trouble, because for the next test we cannot restart Firefox given that the profile is still in use by the former process.
Problem here is that self.returncode internally calls process_handler.proc.poll(), which doesn't care about the outThread, which is still active in those cases, and returns 0.
So this is actually another regression from bug 962495. Process handling fun!!
What we should do is to have a poll() method on the process_handler which will do the right thing. This request has already been filed as bug 965326 and I will work on that now. Once done we can update mozrunner to make use of that new method.
Assignee | ||
Comment 1•11 years ago
|
||
Not sure if Andrew is around today so also adding William as reviewer. Once the dependency bug has been fixed, it will be easier for us to check for the returncode via poll().
Attachment #8370293 -
Flags: review?(wlachance)
Attachment #8370293 -
Flags: review?(ahalberstadt)
Comment 2•11 years ago
|
||
Comment on attachment 8370293 [details] [diff] [review]
Patch v1
Review of attachment 8370293 [details] [diff] [review]:
-----------------------------------------------------------------
Lgtm, thanks!
Attachment #8370293 -
Flags: review?(wlachance)
Attachment #8370293 -
Flags: review?(ahalberstadt)
Attachment #8370293 -
Flags: review+
Assignee | ||
Comment 3•11 years ago
|
||
Now that mozprocess 0.16 has been released (bug 968472) with the new feature included, we can finally land this - after I bumped the mozprocess dep to 0.16.
Assignee | ||
Comment 4•11 years ago
|
||
Jonathan, would you mind to do a quick sanity review?
Attachment #8370293 -
Attachment is obsolete: true
Attachment #8371056 -
Flags: review?(jgriffin)
Comment 5•11 years ago
|
||
Comment on attachment 8371056 [details] [diff] [review]
Patch v1.1
Review of attachment 8371056 [details] [diff] [review]:
-----------------------------------------------------------------
So, we always do a version bump of a package when altering the dependencies; I think we need to do that here as well.
Attachment #8371056 -
Flags: review?(jgriffin) → review-
Comment 6•11 years ago
|
||
Comment on attachment 8371056 [details] [diff] [review]
Patch v1.1
Review of attachment 8371056 [details] [diff] [review]:
-----------------------------------------------------------------
talked on irc, and whimboo convinced me to do this and do a proper version bump later
Attachment #8371056 -
Flags: review- → review+
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #6)
> talked on irc, and whimboo convinced me to do this and do a proper version
> bump later
The version bump will happen if we can be sure that the strange restart test issues in Mozmill tests are finally solved now. I have filed bug 968508.
Assignee | ||
Comment 8•11 years ago
|
||
https://github.com/mozilla/mozbase/commit/3fc539587a5b4155ff8eb56115e7ed5a9702c6c5
Andrei, can you please verify that with latest mozbase master, the problem is fixed in Mozmill? Thanks.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(andrei.eftimie)
Resolution: --- → FIXED
Assignee | ||
Comment 9•11 years ago
|
||
Not sure why but on travis we fail with issues I haven't seen locally. I have to investigate those tomorrow morning.
======================================================================
FAIL: test_stop_process (test_stop.MozrunnerStopTestCase)
Stop the process and test properties
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/build/mozilla/mozbase/mozrunner/tests/test_stop.py", line 19, in test_stop_process
self.assertFalse(self.runner.is_running())
AssertionError: True is not false
======================================================================
FAIL: test_stop_process_custom_signal (test_stop.MozrunnerStopTestCase)
Stop the process via a custom signal and test properties
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/build/mozilla/mozbase/mozrunner/tests/test_stop.py", line 35, in test_stop_process_custom_signal
self.assertFalse(self.runner.is_running())
AssertionError: True is not false
======================================================================
FAIL: test_process_post_stop_via_thread (test_threads.MozrunnerThreadsTestCase)
Stop the runner and try it again with a thread a bit later
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/build/mozilla/mozbase/mozrunner/tests/test_threads.py", line 69, in test_process_post_stop_via_thread
self.assertNotIn(returncode, [None, 0])
AssertionError: None unexpectedly found in [None, 0]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 10•11 years ago
|
||
Status: REOPENED → ASSIGNED
Flags: needinfo?(andrei.eftimie)
Assignee | ||
Comment 11•11 years ago
|
||
So we basically depend at least on bug 967782 and bug 968718 here. I'm not sure what else could come.
Andrew, I hope that you would be fine with the workaround which I will upload in a bit, so that we can get a working mozrunner and our mozmill 2.0.5 release out, which we really need. And once the dependencies of this bug have been fixed, we could fully revert the workarounds.
Assignee | ||
Comment 12•11 years ago
|
||
Not tested across platforms yet. Will ask for review once that has been done.
Attachment #8371056 -
Attachment is obsolete: true
Assignee | ||
Comment 13•11 years ago
|
||
I missed to revert a change which I made for testing purposes. So a single test was failing. The patch as of now passes all the tests.
Attachment #8371338 -
Attachment is obsolete: true
Attachment #8371343 -
Flags: review?(ahalberstadt)
Comment 14•11 years ago
|
||
Comment on attachment 8371338 [details] [diff] [review]
Patch v2
Review of attachment 8371338 [details] [diff] [review]:
-----------------------------------------------------------------
This is still failing for me with the issue from bug 966234
Attachment #8371338 -
Flags: feedback-
Assignee | ||
Comment 15•11 years ago
|
||
Andrei, for me this patch works fine. Please make sure that your machine has been correctly setup, and give me details of what you are running and how. Your comment doesn't tell me anything.
Comment 16•11 years ago
|
||
The testcase from bug 966234 does pass, and I am able to run our tests with a set profile without problems.
But running a testrun still gives the error.
I've disected which dependencies have been updated and the regressor appears to be a combination of ManifestDestiny, mozfile and mozInstall. This looks like a different issue.
Comment 17•11 years ago
|
||
Comment on attachment 8371338 [details] [diff] [review]
Patch v2
Review of attachment 8371338 [details] [diff] [review]:
-----------------------------------------------------------------
All good!
Patched mozmill-automation to require the latest versions of its dependencies
>'mozdownload==1.11.1',
>'mozfile == 1.1',
>'mozinstall == 1.9',
and I can safely run both functional and remote testruns.
Attachment #8371338 -
Flags: feedback- → feedback+
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 8371343 [details] [diff] [review]
Patch v2.1 (with workarounds)
Looks like William and Andrew are out the whole week. Jonathan, would you mind to review? Also adding Joel in case he could do it.
So it might be that we do not need this patch with workarounds if we can get the depending bugs fixed. Might be that we could do that until tomorrow. But for safety I would like to get a review on that one too. Just in case it will not work.
Attachment #8371343 -
Attachment description: Patch v2.1 → Patch v2.1 (with workarounds)
Attachment #8371343 -
Flags: review?(jgriffin)
Attachment #8371343 -
Flags: review?(ahalberstadt)
Attachment #8371343 -
Flags: feedback?(jmaher)
Comment 19•11 years ago
|
||
Comment on attachment 8371343 [details] [diff] [review]
Patch v2.1 (with workarounds)
Review of attachment 8371343 [details] [diff] [review]:
-----------------------------------------------------------------
overall, this is a workaround which appears ok, but i would rather we fix mozprocess (0.17) and go that route.
Attachment #8371343 -
Flags: feedback?(jmaher) → feedback+
Comment 20•11 years ago
|
||
Comment on attachment 8371343 [details] [diff] [review]
Patch v2.1 (with workarounds)
Review of attachment 8371343 [details] [diff] [review]:
-----------------------------------------------------------------
Agree with Joel; if we land this we should file another bug to revert this once mozprocess is fixed.
Attachment #8371343 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 21•11 years ago
|
||
The workaround should not be necessary. I was able to find patches for both the issues I have brought up here. So once the remaining one has been reviewed and possible comments addressed, we can release 0.17. Then this patch will have to be updated to depend on 0.17 instead of 0.16.
Assignee | ||
Comment 22•11 years ago
|
||
Comment on attachment 8371343 [details] [diff] [review]
Patch v2.1 (with workarounds)
All dependencies have been fixed. So the workarounds are not necessary anymore. I will come up with an update for the initial patch with mozprofile=0.17.
Attachment #8371343 -
Attachment is obsolete: true
Assignee | ||
Comment 23•11 years ago
|
||
Final patch which only contains the version bump and the fix for the returncode property.
Andrei, would you mind to do a final pass over if all is fine, and nothing else came up? Thanks.
Attachment #8372112 -
Flags: review+
Attachment #8372112 -
Flags: feedback?(andrei.eftimie)
Comment 24•11 years ago
|
||
Windows looks fine.
I've also used another OSX box (10.8) and on my regular 10.9 box.
This fails for running our restart tests from mozmill-tests
Here are more complete steps. Adding them in this more complete form. Maybe I am missing something...
Steps:
1) installed pip && vitualenv // didn't have these on the 10.8 machine
2) $ virtualenv env
3) $ . env/bin/activate
4) $ git clone http://github.com/mozilla/mozmill
5) $ git clone http://github.com/mozilla/mozbase
6) $ hg clone http://hg.mozilla.org/qa/mozmill-tests
7) $ cd mozmill
8) $ ./setup_development.py
9) $ cd ../mozbase
10)download patch from https://bugzilla.mozilla.org/attachment.cgi?id=8372112 in ../patches
11)$ git checkout -b 967595
12)$ git am ../patches/0001-Bug-967595-mozrunner-Calling-process_handler.proc.po.patch
13)$ ./setup_development.py
14)$ cd ../
15)$ mozmill -m mozmill-tests/firefox/tests/functional/restartTests/manifest.ini -b /Applications/FirefoxNightly.app/ -p profile
This fails.
Assignee | ||
Comment 25•11 years ago
|
||
I ran dozen of tests and I haven't seen this failure at all. So please check your created venv in detail and especially the versions which are used. Keep in mind that running setup_development.py for mozbase after mozmill's one causes mozbase packages to be downloaded from pypi, and not been replaced afterward. This can cause exactly those failures.
Also more comments about this should go onto the Mozmill bug. Here a simple final confirmation should be made.
Comment 26•11 years ago
|
||
Comment on attachment 8372112 [details] [diff] [review]
Patch v3.1
Review of attachment 8372112 [details] [diff] [review]:
-----------------------------------------------------------------
Runs great!
Thanks for the help of IRC.
> Keep in mind that running setup_development.py for mozbase after mozmill's one causes mozbase packages to be downloaded from pypi, and not been replaced afterward. This can cause exactly those failures.
I think this is exactly what has happened.
To confirm now, this is running great on OSX.
Attachment #8372112 -
Flags: feedback?(andrei.eftimie) → feedback+
Assignee | ||
Comment 27•11 years ago
|
||
https://github.com/mozilla/mozbase/commit/2636578efc5b96c813350a93bc4d0cf0c05f0337
An automated test is hard to do in mozrunner. We would need an extension which will restart the application after a couple of seconds. For now I would say we take our Mozmill tests as tests.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•