-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fixing unit tests for Windows #10
Conversation
@@ -7,7 +7,8 @@ | |||
"php": ">=5.4.0", | |||
"evenement/evenement": "~2.0", | |||
"react/event-loop": "0.4.*", | |||
"react/stream": "0.4.*" | |||
"react/stream": "0.4.*", | |||
"phpunit/phpunit": "~4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary, as sebastian/environment
was already a dev dependency. Even if we were to add PHPUnit as a dependency, it should be included under require-dev
. That said, I'm of the opinion that it's redundant though, as we can expect developers and Travis to have it already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, even if sebastian/environment
has a dev dependency on phpunit, dev-dependencies of required packages are not included (only the root package dev-dependencies are included by Composer). Anyway, you are right to say I should have put this in the require-dev
, and anyway, it is a mistake from me. I never intended to commit this change. I'll revert it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if I wasn't clear, but I meant sebastian/environment
was a dev dependency because our tests needed it to create command line strings. It was listed explicitly because some dated versions of PHPUnit (likely what Travis CI had at the time it was first added) did not depend on it, so we couldn't guarantee it would be available. PHPUnit was never a dev dependency because we assume it's available as a global binary (I realize other projects hold different opinions about that, though).
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we expect a test failure on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, just saw your comment in the OP.
See #13. |
Directly related to issue #9 that I opened yesterday, I've been working on Windows PHPUnit tests.
I fixed a number of those tests. Windows has a very very weird way to handle quotes when passed to the command line so I added a few tweaks to the unit tests so that quotes are handled correctly.
I managed to have all existing tests working on Windows.
But...
I added a few more tests that are working flawlessly on Linux, but failing on Windows.
Those tests are generating a big output (1000 / 10000 / 100000 characters).
This is where the fun starts. Output with 1000 works on Windows.
Output of 10000 fails (Windows outputs only 4096 characters)
Output of 100000 succeeds, but.... takes a whooping 32 seconds! (it takes less than 1 second on Linux)
Tests run with PHP 5.6.6.
Any idea of what might go wrong with Windows? I'm pretty sure it's related to Windows implementation of
proc_open
but I don't know how to prove it or how to fix it.