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

Fix: Run commands through PHP binary to ensure they are run on Windows #4335

Merged
merged 4 commits into from Jun 29, 2020

Conversation

localheinz
Copy link
Collaborator

@localheinz localheinz commented Jun 28, 2020

This PR

  • explicitly runs commands through PHP binary to ensure they are run on windows-latest
  • configures git to avoid issues with line endings when running tests on windows-latest
  • adds slashes
  • skips a test related to file permissions on Windows

Fixes #4333.

@localheinz localheinz self-assigned this Jun 28, 2020
@localheinz localheinz force-pushed the fix/windows branch 3 times, most recently from 45ac914 to 2c85b6b Compare June 28, 2020 11:16
@localheinz localheinz changed the title Fix: Run phpunit in debug mode Fix: Run commands through PHP binary to ensure they are run on Windows Jun 28, 2020
@codecov
Copy link

codecov bot commented Jun 28, 2020

Codecov Report

Merging #4335 into 8.5 will increase coverage by 0.43%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                8.5    #4335      +/-   ##
============================================
+ Coverage     84.20%   84.63%   +0.43%     
  Complexity     3936     3936              
============================================
  Files           154      154              
  Lines          9785    10390     +605     
============================================
+ Hits           8239     8794     +555     
- Misses         1546     1596      +50     
Impacted Files Coverage Δ Complexity Δ
src/Framework/SkippedTestCase.php 90.90% <0.00%> (-9.10%) 4.00% <0.00%> (ø%)
src/Framework/IncompleteTestCase.php 90.90% <0.00%> (-9.10%) 4.00% <0.00%> (ø%)
src/Runner/NullTestResultCache.php 66.66% <0.00%> (-8.34%) 6.00% <0.00%> (ø%)
src/Framework/MockObject/Stub/ReturnArgument.php 66.66% <0.00%> (-4.77%) 4.00% <0.00%> (ø%)
src/Framework/Constraint/JsonMatches.php 96.77% <0.00%> (-3.23%) 9.00% <0.00%> (ø%)
src/Framework/Constraint/IsType.php 97.87% <0.00%> (-2.13%) 23.00% <0.00%> (ø%)
src/Util/TestDox/ResultPrinter.php 57.14% <0.00%> (-1.48%) 39.00% <0.00%> (ø%)
...ramework/MockObject/Rule/ConsecutiveParameters.php 80.95% <0.00%> (-0.63%) 14.00% <0.00%> (ø%)
src/Framework/MockObject/InvocationHandler.php 76.92% <0.00%> (-0.13%) 26.00% <0.00%> (ø%)
src/Framework/TestResult.php 71.52% <0.00%> (-0.05%) 173.00% <0.00%> (ø%)
... and 109 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82a15e6...b162354. Read the comment docs.

@@ -141,17 +141,17 @@ jobs:

- name: Install lowest dependencies with composer
if: matrix.dependencies == 'lowest'
run: ./tools/composer update --no-ansi --no-interaction --no-progress --prefer-lowest
run: php ./tools/composer update --no-ansi --no-interaction --no-progress --prefer-lowest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@shivammathur

Do you have an idea why it would be necessary to specify the php binary here for running these commands on windows-latest?

Without it, they are not run, see #4333.

Copy link
Contributor

Choose a reason for hiding this comment

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

@localheinz
#!/usr/bin/env php does not work on Windows, so you will have to call local phar tools with php.

When tools are installed using setup-php on Windows, it creates a batch script to call the tools with php.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you, @shivammathur!

@localheinz localheinz marked this pull request as ready for review June 28, 2020 12:07
@localheinz
Copy link
Collaborator Author

localheinz commented Jun 28, 2020

@sebastianbergmann

Tests are running on windows-latest now, but a bunch of them are failing.

Looks like most of them are related to line-endings.

@localheinz localheinz marked this pull request as draft June 28, 2020 12:08
@dvdoug
Copy link
Contributor

dvdoug commented Jun 28, 2020

Tests are running on windows-latest now, but a bunch of them are failing.

Looks like most of them are related to line-endings.

Most of the failures I think will go away if the checkout uses LF, rather than CRLF. Try the workaround on actions/checkout#38 or forcing LF via .gitattributes.

@localheinz
Copy link
Collaborator Author

Thank you, @dvdoug - that helped! 👍

@localheinz localheinz marked this pull request as ready for review June 29, 2020 05:51
@localheinz
Copy link
Collaborator Author

Does anyone have an idea how to fix the remaining test failures?

I haven't used Windows in the last 10 years.

@shivammathur
Copy link
Contributor

@localheinz To fix XDebugFilterScriptGeneratorTest

$expectedDirectory = __DIR__ . DIRECTORY_SEPARATOR;

- $expectedDirectory = __DIR__ . DIRECTORY_SEPARATOR; 
+ $expectedDirectory = addslashes(__DIR__ . DIRECTORY_SEPARATOR); 

to match

addslashes('%s' . DIRECTORY_SEPARATOR),

Related issue - #3834

@localheinz localheinz force-pushed the fix/windows branch 3 times, most recently from b2ef482 to 6f90153 Compare June 29, 2020 07:01
@localheinz
Copy link
Collaborator Author

localheinz commented Jun 29, 2020

@shivammathur

I applied a fix with 77d7158, but I think it did not help.

@shivammathur
Copy link
Contributor

shivammathur commented Jun 29, 2020

@localheinz Try this, I missed sprintf

$files[] = sprintf(
addslashes('%s' . DIRECTORY_SEPARATOR),
$path
);

- $expectedDirectory = __DIR__ . DIRECTORY_SEPARATOR; 
+ $expectedDirectory = sprintf(addslashes('%s' . DIRECTORY_SEPARATOR), __DIR__); 

@dvdoug
Copy link
Contributor

dvdoug commented Jun 29, 2020

On Windows, chmod() can set the read only flag but that's about it, the filesystem is simply too different in the way it works.

Thus unless you want to try and use shell scripting to set up ACLs, I'd suggest simply skipping PHPUnit\Framework\AssertTest::testAssertFileIsNotReadable on Windows.

@dvdoug
Copy link
Contributor

dvdoug commented Jun 29, 2020

The last failing test tests columns=max which is an implicitly variable number, but has a hard-coded result? I don't think that's a safe expectation as this is now showing.

@localheinz localheinz force-pushed the fix/windows branch 2 times, most recently from fac581f to f49b4d1 Compare June 29, 2020 12:43
@sebastianbergmann sebastianbergmann merged commit 37d02f0 into sebastianbergmann:8.5 Jun 29, 2020
@localheinz localheinz deleted the fix/windows branch June 29, 2020 13:21
@localheinz
Copy link
Collaborator Author

Thank you, @dvdoug, @sebastianbergmann, and @shivammathur!

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

4 participants