Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows test suite compatibility #13

Closed
wants to merge 3 commits into from
Closed

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Mar 9, 2015

This PR is a rebased version of #10 with additional cleanup.

// Note: very strange behaviour of Windows (PHP 5.5.6):
// on a 1000 long string, Windows succeeds.
// on a 10000 long string, Windows fails to output anything.
// On a 100000 long string, it takes a lot of time but succeeds.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moufmouf: Per your explanation in #10 and the above comments, this test fails on Windows. Does the workaround you're proposing in #11 fix this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmikola: Yes, this test fails on Windows... In PHP > 5.6.3, it fails. In PHP < 5.6.3, it locks and the test never halts, which is even worst. #11 fixes this test case.

@jmikola
Copy link
Member Author

jmikola commented Mar 9, 2015

@moufmouf: I'd appreciate it if you could run these test suite changes on Windows and report back whether they succeed. Based on earlier discussion with @auroraeosrose, I was inclined to make bypass_shell default to true. I also refactored the command-line preparation to ensure all tests are preparing the command-line consistently.

$output .= $data;
});
$process->stderr->on('data', function ($data) use (&$error) {
$error .= $data;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moufmouf: Why was the stderr check only added to this test case? Was there a case where it failed on Windows and you caught some output here?

@moufmouf
Copy link

@jmikola Here is the result of the test suite running on PHP 5.6.6 (it's a bit long, I put the raw output so you can see what is happening):

PHPUnit 4.5.0 by Sebastian Bergmann and contributors.

Configuration read from D:\Projects\harmony\child-process-pr-win-test-compat\php
unit.xml.dist

.S.SSSSSSSSSSSSSS.S.SSSSSSSSSSSSSS.E.ESS..ESESS.FF

Time: 33.21 seconds, Memory: 5.00Mb

There were 4 errors:

1) React\Tests\ChildProcess\StreamSelectLoopProcessTest::testSetEnhanceSigchildC
ompatibilityCannotBeCalledIfProcessIsRunning
proc_open(): CreateProcess failed, error code - 2

D:\Projects\harmony\child-process-pr-win-test-compat\src\Process.php:98
D:\Projects\harmony\child-process-pr-win-test-compat\tests\AbstractProcessTest.p
hp:31

2) React\Tests\ChildProcess\StreamSelectLoopProcessTest::testIsRunning
proc_open(): CreateProcess failed, error code - 2

D:\Projects\harmony\child-process-pr-win-test-compat\src\Process.php:98
D:\Projects\harmony\child-process-pr-win-test-compat\tests\AbstractProcessTest.p
hp:47

3) React\Tests\ChildProcess\StreamSelectLoopProcessTest::testStartAndAllowProces
sToExitSuccessfullyUsingEventLoop
proc_open(): CreateProcess failed, error code - 2

D:\Projects\harmony\child-process-pr-win-test-compat\src\Process.php:98
D:\Projects\harmony\child-process-pr-win-test-compat\tests\AbstractProcessTest.p
hp:173
D:\Projects\harmony\child-process-pr-win-test-compat\vendor\react\event-loop\Tim
er\Timers.php:90
D:\Projects\harmony\child-process-pr-win-test-compat\vendor\react\event-loop\Str
eamSelectLoop.php:177
D:\Projects\harmony\child-process-pr-win-test-compat\tests\AbstractProcessTest.p
hp:176

4) React\Tests\ChildProcess\StreamSelectLoopProcessTest::testStartAlreadyRunning
Process
proc_open(): CreateProcess failed, error code - 2

D:\Projects\harmony\child-process-pr-win-test-compat\src\Process.php:98
D:\Projects\harmony\child-process-pr-win-test-compat\tests\AbstractProcessTest.p
hp:222

--

There were 2 failures:

1) React\Tests\ChildProcess\StreamSelectLoopProcessTest::testProcessWithFixedOut
putSize with data set #1 (10000, 5)
Failed asserting that 4096 matches expected 10002.

D:\Projects\harmony\child-process-pr-win-test-compat\tests\AbstractProcessTest.p
hp:348

2) React\Tests\ChildProcess\StreamSelectLoopProcessTest::testProcessWithFixedOut
putSize with data set #2 (100000, 5)
Process took longer than expected.
Failed asserting that 32.302847146987915 is equal to 5 or is less than 5.

D:\Projects\harmony\child-process-pr-win-test-compat\tests\AbstractProcessTest.p
hp:350

FAILURES!
Tests: 45, Assertions: 34, Failures: 2, Errors: 4, Skipped: 35.

Note: on PHP 5.5.12, the unit test simply locks due to a PHP bug (that was fixed in PHP 5.5.18 and PHP 5.6.3)

Here is the output (until it locks and stops responding):

Configuration read from D:\Projects\harmony\child-process-pr-win-test-compat\php
unit.xml.dist

.S.SSSSSSSSSSSSSS.S.SSSSSSSSSSSSSS.E.ESS..ESESS..

@jmikola
Copy link
Member Author

jmikola commented Mar 11, 2015

@moufmouf: If I understand correctly, the testProcessWithFixedOutputSize cases are expected to fail. Were the earlier four errors expected? I didn't see them mentioned in your previous PR.

CreateProcess failed, error code - 2 seems terribly vague, but it appears to be due to a missing file path (see: mikehaertl/phpwkhtmltopdf#8 (comment)). I assume this is because those tests refer to the sleep and echo commands. Is it possible that enabling the bypass_shell option for proc_open() broke that and they were previously emulated by cmd.exe?

moufmouf and others added 3 commits March 8, 2016 16:26
Cleans up some formatting, uses a more precise check for execution duration, and applies more reliable escaping of Windows command-line parameters.
@jmikola jmikola force-pushed the windows-test-compat branch from b083aeb to df0e00b Compare March 8, 2016 21:26
@clue
Copy link
Member

clue commented Jan 13, 2018

Thank you for digging into this and filing this PR! 👍

I still think that getting this feature in makes perfect sense and I'm looking forward to a future update here as per #9 (comment)!

Given that this is currently a WIP and hasn't seen any updates in a while and I don't see it's likely this will get traction any time soon, I will have to close this for now. Please don't see this as discouraging, we would still love to get this feature in and very much appreciate your effort! You can simply complete this feature and update this PR and we're happy to reopen this 👍

Thank you for your effort!

@clue clue closed this Jan 13, 2018
@clue clue deleted the windows-test-compat branch January 13, 2018 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants