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

Get PHPUnit running on Windows #2555

Merged
merged 2 commits into from
Feb 23, 2024
Merged

Get PHPUnit running on Windows #2555

merged 2 commits into from
Feb 23, 2024

Conversation

MatmaRex
Copy link
Contributor

With these patches, vendor/bin/phpunit runs to completion on my Windows machine, without crashing the test runner or hanging forever.

Tests: 3550, Assertions: 6277, Errors: 188, Failures: 410, Skipped: 5, Risky: 1.

:)

(the number of errors varies slightly, something must be flapping)

@MatmaRex
Copy link
Contributor Author

Huh, that's weird.


private function makeEnvDumpProcess(): ProcessBuilder
{
return ProcessBuilder::create([PHP_BINARY, '-r', 'var_dump($_ENV);']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

confused why this is necesary when we use mergeParentEnv - do processes not receive env vars on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do, but I'm afraid I don't really understand the question. ProcessBuilder (and Process) override that behavior for some reason, and this is a test for it.

Copy link
Collaborator

@dantleech dantleech Feb 23, 2024

Choose a reason for hiding this comment

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

right, but the test is "is the env passed to the child process" but here we create the child PHP process and .... - which invalidates the test?

Copy link
Collaborator

@dantleech dantleech Feb 23, 2024

Choose a reason for hiding this comment

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

ok i see, ignore me, export is not available on Windows.

@MatmaRex
Copy link
Contributor Author

I figured out why it wasn't working, apparently $_ENV isn't always set (https://www.php.net/manual/en/reserved.variables.environment.php#98113). Thanks for the pointer to getenv() @mamazu (I saw your comment before it was deleted).

@MatmaRex
Copy link
Contributor Author

I am now even more confused by the new failure, which is in a test case I didn't touch. Is this a known flaky test?

There was 1 failure:

1) Phpactor\Indexer\Tests\Adapter\Tolerant\TolerantIndexBuilderTest::testDoesNotRemoveExisting
Failed asserting that actual size 4 matches expected size 3.

/home/runner/work/phpactor/phpactor/lib/Indexer/Tests/Adapter/IndexBuilderTestCase.php:579

Shells other than Bash may not have an `export` builtin command
(in particular it doesn't exist on Windows). Shells are icky anyway.
Let's make our own in one line of PHP.

I'm doing this as the first patch towards making PHPUnit pass on
Windows, because trying to run a command that doesn't exist triggers
a really unpleasant bug in amphp/process 1.x on Windows, where it
hangs forever: amphp/process#71

(I had to move `$process->getStdout()` before `$process->join()`,
otherwise that would also hang forever. No idea why.)
Otherwise it tries to read from standard input, which is blocking on
Windows.
@dantleech
Copy link
Collaborator

dantleech commented Feb 23, 2024

Is this a known flaky test?

yes it is, you can ignore it, maybe it will be fixed one year :)

@dantleech dantleech merged commit 0c4f38c into phpactor:master Feb 23, 2024
10 checks passed
@MatmaRex MatmaRex deleted the phpunit branch February 24, 2024 15:21
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.

None yet

2 participants