[mozprocess] Enable mozprocess unit tests on Windows in automation

RESOLVED FIXED in Firefox 60

Status

Testing
Mozbase
P1
normal
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: nodeless, Assigned: whimboo)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [mozprocess])

MozReview Requests

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

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 774538 [details]
0001-Fix-failing-mozprocess-tests-on-windows.patch

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'.
(Reporter)

Comment 1

5 years ago
Created attachment 774562 [details] [diff] [review]
Updated patch

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

Updated

5 years ago
Summary: Fix failing mozprocess tests on windows → [mozprocess] Fix failing mozprocess tests on windows
Whiteboard: [mozprocess]

Comment 3

3 months ago
Mass closing bugs with no activity in 2+ years. If this bug is important to you, please re-open.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → WONTFIX
(Assignee)

Comment 4

2 months ago
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 → ---
(Assignee)

Comment 5

2 months ago
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
(Assignee)

Comment 6

2 months ago
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
(Assignee)

Comment 7

2 months ago
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.
(Assignee)

Updated

2 months ago
Depends on: 1436857
(Assignee)

Comment 8

2 months ago
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!
(Assignee)

Comment 9

2 months ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #774562 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

2 months ago
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.
(Assignee)

Comment 17

2 months ago
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.
(Assignee)

Comment 18

2 months ago
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.
(Assignee)

Comment 19

2 months ago
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.
(Assignee)

Updated

2 months ago
Blocks: 1438009
(Assignee)

Comment 20

2 months ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8950254 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8950253 - Flags: review?(ahalberstadt)
Attachment #8950389 - Flags: review?(ahalberstadt)
Attachment #8950390 - Flags: review?(ahalberstadt)
Attachment #8950255 - Flags: review?(ahalberstadt)
(Assignee)

Comment 25

2 months ago
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.
(Assignee)

Comment 26

2 months ago
After the TC outage for Windows test workers the test finally run and all succeed.

Comment 27

2 months ago
mozreview-review
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 28

2 months ago
mozreview-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 29

2 months ago
mozreview-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 30

2 months ago
mozreview-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+
(Assignee)

Comment 31

2 months ago
mozreview-review-reply
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 32

2 months ago
mozreview-review-reply
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.
(Assignee)

Comment 33

2 months ago
mozreview-review-reply
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.
(Assignee)

Comment 34

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 39

2 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 43

2 months ago
mozreview-review-reply
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

2 months ago
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.

Comment 48

2 months ago
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

Comment 49

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61753b98b547
https://hg.mozilla.org/mozilla-central/rev/720afff2d5e5
https://hg.mozilla.org/mozilla-central/rev/81aafa6d326a
https://hg.mozilla.org/mozilla-central/rev/02896a1245a5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago2 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.