Closed Bug 892902 Opened 13 years ago Closed 8 years ago

[mozprocess] Enable mozprocess unit tests on Windows in automation

Categories

(Testing :: Mozbase, defect, P1)

Product:
Testing
Component:
Mozbase
Type:
defect

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: nodeless, Assigned: whimboo)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [mozprocess])

Attachments

(4 files, 3 obsolete files)

Bug 892902 - [mozprocess] Remove dependency of psutil for pid exists check.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 892902 - [mozprocess] Enable unit tests on Windows.
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 892902 - [mozprocess] Returncode for kill() on Windows has to be set from wait().
59 bytes, text/x-review-board-request
ahal
: review+
Details
Bug 892902 - [mozprocess] Ensure that process returncodes are positive on Windows.
59 bytes, text/x-review-board-request
ahal
: review+
Details
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release) Build ID: 20130618035212 Steps to reproduce: mozprocess tests fail on Windows. I did some work on the tests and mozprocess itself to fix some bugs so that all the tests pass now (tested on win7 and debian), and the tearDown step succeeds. I am not able to test the compilation step (make_proclaunch()) on Windows, because I don't have my environment setup properly with the mozilla build tools. I hacked up the Makefile so it works under cygwin, then I run the mozprocess tests in windows witih 'python tests/test_mozprocess.py'.
Attached patch Updated patch (obsolete) — — Splinter Review
test_process_waittimeout did not kill its test process, causing 'access denied' error on windows when removing test binary. I originally "fixed" this by adding a call to sleep() before os.remove(). Updated patch to include the proper fix (i.e., have this testcase kill its process when it's done).
Attachment #774538 - Attachment is obsolete: true
Using this as a follow up to the issue noted in comments 26 and 27 on https://bugzilla.mozilla.org/show_bug.cgi?id=778267 . Basically mozprocess tests fail on windows because mozprocess returns code 572 (0x23c) upon termination of a process, which is windows for[1] ERROR_CONTROL_C_EXIT, which is not what we are actually doing. This is not consistent with what it's unix and linux counterparts return. As :ctalbert rightly points out we should decide what the proper return code on windows should be when we terminate a process, and make changes to mozprocess and it's tests accordingly. [1]http://msdn.microsoft.com/en-us/library/windows/desktop/ms681388(v=vs.85).aspx
Summary: Fix failing mozprocess tests on windows → [mozprocess] Fix failing mozprocess tests on windows
Whiteboard: [mozprocess]
Mass closing bugs with no activity in 2+ years. If this bug is important to you, please re-open.
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
This is clearly NOT a wontfix. Currently we run no mozprocess tests on Windows at all due to those problems.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---
Via bug 1435820 I'm removing the C implementation of both the proc launcher and iniparser. Both tools have been rewritten in Python but were still lingering around in the mozprocess test folder. Once this is done I will try to enable the mozprocess tests in automation.
Blocks: 1434878
Depends on: 1435820
The mozprocess unit tests currently cannot be run on Windows due to the following failure: 08:40:26 INFO - z:\build\build\src\testing\mozbase\mozprocess\tests\test_kill.py 08:40:26 INFO - Traceback (most recent call last): 08:40:26 INFO - File "z:\build\build\src\testing\mozbase\mozprocess\tests\test_kill.py", line 8, in <module> 08:40:26 INFO - import proctest 08:40:26 INFO - File "z:\build\build\src\build\mach_bootstrap.py", line 363, in __call__ 08:40:26 INFO - module = self._original_import(name, globals, locals, fromlist, level) 08:40:26 INFO - File "z:\build\build\src\testing\mozbase\mozprocess\tests\proctest.py", line 6, in <module> 08:40:26 INFO - import psutil 08:40:26 INFO - File "z:\build\build\src\build\mach_bootstrap.py", line 363, in __call__ 08:40:26 INFO - module = self._original_import(name, globals, locals, fromlist, level) 08:40:26 INFO - File "z:\build\build\src\third_party\python\psutil\psutil\__init__.py", line 110, in <module> 08:40:26 INFO - from . import _pswindows as _psplatform 08:40:26 INFO - File "z:\build\build\src\build\mach_bootstrap.py", line 363, in __call__ 08:40:26 INFO - module = self._original_import(name, globals, locals, fromlist, level) 08:40:26 INFO - File "z:\build\build\src\third_party\python\psutil\psutil\_pswindows.py", line 16, in <module> 08:40:26 INFO - from . import _psutil_windows as cext 08:40:26 INFO - ImportError: cannot import name _psutil_windows 08:40:26 WARNING - TEST-UNEXPECTED-FAIL | No test output (missing mozunit.main() call?): z:\build\build\src\testing\mozbase\mozprocess\tests\test_kill.py I will have a look at this tomorrow.
Assignee: nobody → hskupin
Status: REOPENED → ASSIGNED
Priority: -- → P1
Summary: [mozprocess] Fix failing mozprocess tests on windows → [mozprocess] Enable mozprocess unit tests on Windows in automation
Here what Ted mentioned on IRC: @ted> it has a C module, and those are a PITA to build on windows 22:27:32 <@ted> because they have to be built with a version of MSVC that matches what python was built with 22:27:42 <@ted> and for python 2.x that's like VC9 22:28:28 <@ted> i assume it should be possible to publish a psutil wheel, and maybe there already is one? 22:29:12 <@ted> https://pypi.python.org/pypi/psutil shows wheels of the latest release, certainly 22:31:20 <ahal> we'd need still need the source for linux/mac + 2 windows wheels per python version we want to support So we would have to find a way to get those platform dependent packages listed under third_party, or we install it independently from PyPI for mozprocess tests. I will file a bug.
Depends on: 1436857
I got the tests running locally and it looks great. There is only a single failure: > 3:21.75 self.assertLess(returncode2, 0, > 3:21.75 > 'Negative returncode expected, got "%s"' % returncode2) > 3:21.75 E AssertionError: Negative returncode expected, got "572" > 3:21.75 > 3:21.75 testing\mozbase\mozprocess\tests\test_wait.py:95: AssertionError All other test modules results in a pass!
We cannot depend on the psutil package due to build config issues. It will be easier to just rewrite the code and make use of ctypes on Windows to check if a pid exists.
No longer depends on: 1436857
Attachment #774562 - Attachment is obsolete: true
I got all mozprocess unit tests to pass on Windows now. I triggered a try with a couple of other test jobs given that I had to make some process handler changes specifically for Windows.
Everything looks fine except Marionette and Firefox UI tests on Windows 7. Surprisingly Windows 10 is fine too. So I wonder what makes those two versions different here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbcbabda181b The failures seem to happen in `is_running()`, which falsely reports that the process is still active while it is not.
So what's broken here is what I have already been started to work on bug 1433905. To not run into the circular dependency again I would suggest to implement a workaround in Marionette for now, which will continue to use `is_running()` for Android, but uses `self.runner.process_handler.pid_exists()` otherwise. I hope that this should get us going so I can continue to fix all the other problems.
Quick note... this doesn't work. I might want to revert the Windows related changes in process handler and file that as a separate bug. Lets get test jobs working first.
Blocks: 1438009
Ok, so I found it. The reason for the failure is the change to return `None` in-case the returncode is `STILL_ACTIVE`. It's pretty bad that we cannot change that now, but it is better suited for a new bug. So I filed bug 1438009 to take care of it after this patch landed.
Attachment #8950254 - Attachment is obsolete: true
Attachment #8950253 - Flags: review?(ahalberstadt)
Attachment #8950389 - Flags: review?(ahalberstadt)
Attachment #8950390 - Flags: review?(ahalberstadt)
Attachment #8950255 - Flags: review?(ahalberstadt)
The currently running try build is just for sanity to see that also in automation all is fine. Given that I was able to replicate the failures in my Win7 VM and got them fixed, I'm fairly sure that the try build should also succeed. As such I already requested review.
After the TC outage for Windows test workers the test finally run and all succeed.
Comment on attachment 8950253 [details] Bug 892902 - [mozprocess] Remove dependency of psutil for pid exists check. https://reviewboard.mozilla.org/r/219500/#review226186 Looks good, though please fix the issue and remove the six dependency. ::: testing/mozbase/mozprocess/mozprocess/processhandler.py:899 (Diff revision 2) > + # re-raise for any other type of exception > + exc, msg, tb = sys.exc_info() > + reraise(exc, msg, tb) Can't we just use a blank `raise` here and avoid the dependency on six?
Attachment #8950253 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8950389 [details] Bug 892902 - [mozprocess] Returncode for kill() on Windows has to be set from wait(). https://reviewboard.mozilla.org/r/219608/#review226190 ::: testing/mozbase/mozprocess/mozprocess/processhandler.py (Diff revision 2) > - except Exception: > - traceback.print_exc() > - raise OSError("Could not terminate process") Shouldn't we leave this `except` clause and move `self._cleanup()` out of the finally block and into there? except Exception: self._cleanup() raise Or is cleanup not important in this case?
Attachment #8950389 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8950390 [details] Bug 892902 - [mozprocess] Ensure that process returncodes are positive on Windows. https://reviewboard.mozilla.org/r/219610/#review226194
Attachment #8950390 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8950255 [details] Bug 892902 - [mozprocess] Enable unit tests on Windows. https://reviewboard.mozilla.org/r/219504/#review226196 Thank you so much for doing this, it was long overdue!
Attachment #8950255 - Flags: review?(ahalberstadt) → review+
Comment on attachment 8950253 [details] Bug 892902 - [mozprocess] Remove dependency of psutil for pid exists check. https://reviewboard.mozilla.org/r/219500/#review226186 > Can't we just use a blank `raise` here and avoid the dependency on six? That way we will loose the original traceback from raising the exception in winprocess.py. I like to keep the original traceback for better investigation of problems, and also wouldn't worry that much about the six depencency given that we also use it for various other mozbase modules. You think we don't need that?
Comment on attachment 8950253 [details] Bug 892902 - [mozprocess] Remove dependency of psutil for pid exists check. https://reviewboard.mozilla.org/r/219500/#review226186 > That way we will loose the original traceback from raising the exception in winprocess.py. I like to keep the original traceback for better investigation of problems, and also wouldn't worry that much about the six depencency given that we also use it for various other mozbase modules. > > You think we don't need that? I could be mistaken, but I'm pretty sure a blank `raise` will preserve the original traceback.
Comment on attachment 8950389 [details] Bug 892902 - [mozprocess] Returncode for kill() on Windows has to be set from wait(). https://reviewboard.mozilla.org/r/219608/#review226190 > Shouldn't we leave this `except` clause and move `self._cleanup()` out of the finally block and into there? > > except Exception: > self._cleanup() > raise > > Or is cleanup not important in this case? Hm, interesting. I wonder why we don't have that for the case above when calling `TerminateJobObject()`. So we should have it for both or none of them. I will have a look.
Comment on attachment 8950253 [details] Bug 892902 - [mozprocess] Remove dependency of psutil for pid exists check. https://reviewboard.mozilla.org/r/219500/#review226186 > I could be mistaken, but I'm pretty sure a blank `raise` will preserve the original traceback. Yes, that works. I will make that change.
Comment on attachment 8950389 [details] Bug 892902 - [mozprocess] Returncode for kill() on Windows has to be set from wait(). https://reviewboard.mozilla.org/r/219608/#review226190 > Hm, interesting. I wonder why we don't have that for the case above when calling `TerminateJobObject()`. So we should have it for both or none of them. I will have a look. I added a try/except around both calls.
Comment on attachment 8950389 [details] Bug 892902 - [mozprocess] Returncode for kill() on Windows has to be set from wait(). https://reviewboard.mozilla.org/r/219608/#review226190 > I added a try/except around both calls. I actually decided to keep the exception type as raised by that code. Not sure which consumers might depend on it.
I got an additional r+ from Andrew via IRC for the last updated patch. Given that the try job all passed I'm going to push this patch now.
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/61753b98b547 [mozprocess] Remove dependency of psutil for pid exists check. r=ahal https://hg.mozilla.org/integration/autoland/rev/720afff2d5e5 [mozprocess] Returncode for kill() on Windows has to be set from wait(). r=ahal https://hg.mozilla.org/integration/autoland/rev/81aafa6d326a [mozprocess] Ensure that process returncodes are positive on Windows. r=ahal https://hg.mozilla.org/integration/autoland/rev/02896a1245a5 [mozprocess] Enable unit tests on Windows. r=ahal
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: