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

[PHPUnit-Bridge] Fail-fast in simple-phpunit if one of the passthru() commands fails #35254

Merged
merged 1 commit into from Jan 9, 2020

Conversation

@mpdude
Copy link
Contributor

mpdude commented Jan 8, 2020

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

Some commands executed by the simple-phpunit script are not checked for success. For example here, Composer fails with the message

  [InvalidArgumentException]                                                   
  Could not find package phpunit/phpunit with version 7.5.* in a version inst  
  allable using your PHP version 7.0.25.                                       

Yet, the simple-phpunit script happily continues, going over failing chdir(), file_get_contents() and include() calls and eventually returns a successful 0 exit code. So CI tests look OK when in fact PHPUnit was not even downloaded.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 8, 2020

We still fox bugs in lower branches so 3.4 might be a fit.

@mpdude mpdude force-pushed the mpdude:phpunit-bridge-fail-passthru branch from 94ba72a to 6635484 Jan 8, 2020
@mpdude mpdude changed the base branch from master to 3.4 Jan 8, 2020
@mpdude mpdude changed the title Fail in simple-phpunit if one of the passthru() commands fails [PHPUnit-Bridge] Fail-fast in simple-phpunit if one of the passthru() commands fails Jan 8, 2020
@mpdude

This comment has been minimized.

Copy link
Contributor Author

mpdude commented Jan 8, 2020

Rebased onto 3.4

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jan 8, 2020
@mpdude

This comment has been minimized.

Copy link
Contributor Author

mpdude commented Jan 8, 2020

Updated

@@ -15,6 +15,13 @@

error_reporting(-1);

$passthru_or_fail = function ($command) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 8, 2020

Member

we use camel case :)
but $passthru would be fine as a name to me

This comment has been minimized.

Copy link
@mpdude

mpdude Jan 8, 2020

Author Contributor

Will change, but prefer to keep the orFail to make clear there's a difference to plain passthru()

$passthru_or_fail = function ($command) {
passthru($command, $return);
if ($return !== 0) {
exit(99);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 8, 2020

Member
if ($return) {
    exit($return);
}

This comment has been minimized.

Copy link
@mpdude

mpdude Jan 8, 2020

Author Contributor

Will change

@@ -73,29 +80,29 @@ if (!file_exists("$PHPUNIT_DIR/phpunit-$PHPUNIT_VERSION/phpunit") || md5_file(__
@mkdir($PHPUNIT_DIR, 0777, true);
chdir($PHPUNIT_DIR);
if (file_exists("phpunit-$PHPUNIT_VERSION")) {
passthru(sprintf('\\' === DIRECTORY_SEPARATOR ? 'rmdir /S /Q %s > NUL': 'rm -rf %s', "phpunit-$PHPUNIT_VERSION.old"));
$passthru_or_fail(sprintf('\\' === DIRECTORY_SEPARATOR ? 'rmdir /S /Q %s > NUL': 'rm -rf %s', "phpunit-$PHPUNIT_VERSION.old"));

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 8, 2020

Member

this can and should fail silently by design, isn't it?

This comment has been minimized.

Copy link
@mpdude

mpdude Jan 8, 2020

Author Contributor

What is it trying to do here at all?

remove the -.old version if it is there, then rename phpunit-$VERSION to phpunit-$VERSION.old, and then try removing that again?

Doesn't that effectively remove both the .old and current one?

rename("phpunit-$PHPUNIT_VERSION", "phpunit-$PHPUNIT_VERSION.old");
passthru(sprintf('\\' === DIRECTORY_SEPARATOR ? 'rmdir /S /Q %s': 'rm -rf %s', "phpunit-$PHPUNIT_VERSION.old"));
$passthru_or_fail(sprintf('\\' === DIRECTORY_SEPARATOR ? 'rmdir /S /Q %s': 'rm -rf %s', "phpunit-$PHPUNIT_VERSION.old"));

This comment has been minimized.

Copy link
@nicolas-grekas

This comment has been minimized.

Copy link
@mpdude

mpdude Jan 8, 2020

Author Contributor

see above

@nicolas-grekas nicolas-grekas force-pushed the mpdude:phpunit-bridge-fail-passthru branch from fbbb23d to 576e185 Jan 9, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 9, 2020

Thank you @mpdude.

nicolas-grekas added a commit that referenced this pull request Jan 9, 2020
… passthru() commands fails (mpdude)

This PR was squashed before being merged into the 3.4 branch.

Discussion
----------

[PHPUnit-Bridge] Fail-fast in simple-phpunit if one of the passthru() commands fails

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

Some commands executed by the `simple-phpunit` script are not checked for success. For example [here](https://travis-ci.org/twigphp/Twig/jobs/634110681), Composer fails with the message

```
  [InvalidArgumentException]
  Could not find package phpunit/phpunit with version 7.5.* in a version inst
  allable using your PHP version 7.0.25.
```

Yet, the `simple-phpunit` script happily continues, going over failing `chdir()`, `file_get_contents()` and `include()` calls and eventually returns a successful `0` exit code. So CI tests look OK when in fact PHPUnit was not even downloaded.

Commits
-------

576e185 [PHPUnit-Bridge] Fail-fast in simple-phpunit if one of the passthru() commands fails
@nicolas-grekas nicolas-grekas merged commit 576e185 into symfony:3.4 Jan 9, 2020
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
@mpdude mpdude deleted the mpdude:phpunit-bridge-fail-passthru branch Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.