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(test): exit code of lime test -- add proof of current behaviour #371

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
5 changes: 5 additions & 0 deletions .php-cs-fixer.dist.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
->in(__DIR__.'/lib')
->in(__DIR__.'/data/bin')
->in(__DIR__.'/test')
->in(__DIR__.'/test/unit/vendor')
->append([__FILE__])
// Exclude PHP classes templates/generators, which are not valid PHP files
->exclude('task/generator/skeleton/')
Expand All @@ -21,6 +22,9 @@
// Exclude generated files (single files)
->notPath('unit/config/fixtures/sfDefineEnvironmentConfigHandler/prefix_result.php')
->notPath('unit/config/fixtures/sfFilterConfigHandler/result.php')

// Exclude files with expected parsing error
->notPath('lime/fixtures/pass_with_one_parse_error.php')
;

$config = new PhpCsFixer\Config();
Expand All @@ -29,6 +33,7 @@
'@PhpCsFixer' => true,
'@Symfony' => true,
'array_syntax' => ['syntax' => 'short'],
'php_unit_method_casing' => false,
])
->setCacheFile('.cache/php-cs-fixer.cache')
->setFinder($finder)
Expand Down
20 changes: 16 additions & 4 deletions lib/vendor/lime/lime.php
Original file line number Diff line number Diff line change
Expand Up @@ -952,8 +952,7 @@ function lime_shutdown()
);

ob_start();
// see http://trac.symfony-project.org/ticket/5437 for the explanation on the weird "cd" thing
passthru(sprintf('cd & %s %s 2>&1', escapeshellarg($this->php_cli), escapeshellarg($test_file)), $return);
$return = $this->executePhpFile($test_file);
ob_end_clean();
unlink($test_file);

Expand Down Expand Up @@ -1123,6 +1122,20 @@ public function get_failed_files()
{
return isset($this->stats['failed_files']) ? $this->stats['failed_files'] : array();
}

/**
* The command fails if the path to php interpreter contains spaces.
* The only workaround is adding a "nop" command call before the quoted command.
* The weird "cd &".
*
* see http://trac.symfony-project.org/ticket/5437
*/
public function executePhpFile(string $phpFile): int
{
passthru(sprintf('cd & %s %s 2>&1', escapeshellarg($this->php_cli), escapeshellarg($phpFile)), $return);

return $return;
}
}

class lime_coverage extends lime_registration
Expand Down Expand Up @@ -1186,8 +1199,7 @@ public function process($files)
EOF;
file_put_contents($tmp_file, $tmp);
ob_start();
// see http://trac.symfony-project.org/ticket/5437 for the explanation on the weird "cd" thing
Copy link
Collaborator

Choose a reason for hiding this comment

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

[PATCH] lime.php not working on windows when there are spaces in php interpreter path

lime.php, line 541

passthru(sprintf('%s "%s" 2>&1', $this->php_cli, $file), $return);

the command fails if the path to php interpreter contains spaces, this obviously prevent all test to run.

In order to work with executables in directories with spaces it should be

passthru(sprintf('"%s" "%s" 2>&1', $this->php_cli, $file), $return);

but this fails too on windows: system/exec/passthru all fail when more than two arguments are quoted (it seems a php bug but I couldn't find any reference apart from some comments on the system function page)

In order to work on windows it should be written as

passthru(sprintf('""%s" "%s"" 2>&1', $this->php_cli, $file), $return);

this works on windows but still fails on linux (note the double "double quotes": I need to quote the entire command not just the executable and the argument)

The only workaround I could find was something I found on the comments of the system function that is adding a "nop" command call before the quoted command

passthru(sprintf('cd & "%s" "%s" 2>&1', $this->php_cli, $file), $return);

passthru(sprintf('cd & %s %s 2>&1', escapeshellarg($this->harness->php_cli), escapeshellarg($tmp_file)), $return);
$return = $this->harness->executePhpFile($tmp_file);
$retval = ob_get_clean();

if (0 != $return) // test exited without success
Expand Down
7 changes: 7 additions & 0 deletions test/unit/vendor/lime/fixtures/failed.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test();

$test->is(false, true);
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test(1);

$test->is(false, true);
$test->is(true, true);
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test(2);

$test->is(false, true);
7 changes: 7 additions & 0 deletions test/unit/vendor/lime/fixtures/pass.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test();

$test->is(true, true);
13 changes: 13 additions & 0 deletions test/unit/vendor/lime/fixtures/pass_with_one_error.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

error_reporting(E_USER_ERROR);

$test = new lime_test(null, [
'error_reporting' => true,
]);

trigger_error('some user error message', E_USER_ERROR);

$test->is(true, true);
7 changes: 7 additions & 0 deletions test/unit/vendor/lime/fixtures/pass_with_one_parse_error.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test();

parse_error
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should be excluded from everywhere that parses php files. At this moment php-cs-fixer, but later phpstan, rector etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

php-cs-fixer exclude all files that contain "vendor" on its path.

This means that the directory "test/unit/vendor" is ignored.

Fixing that, will requires a structural correction, that is out of scope of this PR.

@connorhu What can I do on the scope of this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, you are right! For the other tools, it was just a reminder to myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test();

throw new LogicException('some exception message');
$test->is(true, true);
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test(1);

$test->is(true, true);
$test->is(true, true);
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test(2);

$test->is(true, true);
7 changes: 7 additions & 0 deletions test/unit/vendor/lime/fixtures/skip.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test();

$test->skip('some skip message');
7 changes: 7 additions & 0 deletions test/unit/vendor/lime/fixtures/skip_with_plan.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

require_once __DIR__.'/../../../../bootstrap/unit.php';

$test = new lime_test($plan = 2);

$test->skip('some skip message', $plan);
Loading