amphp / process

An async process dispatcher for Amp.
MIT License
233 stars 30 forks source link

Improve code coverage #17

Closed peter279k closed 6 years ago

peter279k commented 7 years ago

Changed log

peter279k commented 7 years ago

Hi @kelunik, thank you for your review. I have pushed the latest commit. Please review this.

Thanks.

peter279k commented 7 years ago

Ok, I've modified the CMD_PROCESS variable value globally.

Thank you for this review.

kelunik commented 7 years ago

Sorry for not merging this earlier. Could you rebase your changes onto the current master branch and fix the tests for the minor BC breaks mentioned in the release notes?

peter279k commented 6 years ago

Hi @kelunik , thank you for your reply. I will do this and I'm sorry for that I'm delay for this notification.

Thanks.

peter279k commented 6 years ago

According to this issue, just wait for the Composer fix this bug. Then the Travis build will be worked successfully.

peter279k commented 6 years ago

When I track the Travis build during testing PHP 7.0, the options-resolver version which is the PHP-CS-Fixer dependencies is not correct. And I remove the --ignore-platform-reqs option, the Travis build is successful. I think this PR is not problem at this time so I remove this option.

trowski commented 6 years ago

@peter279k Removing --ignore-platform-reqs is not really correct, as that causes the nightly build to fail. We're trying to decide what to do with the travis build so we don't have to keep changing it due to PHP-CS-Fixer. I'll just remove that change when I squash this.

peter279k commented 6 years ago

Hi @trowski, thank you for your explanation. When adding the ignore-platform-reqs option, the option-resolver dependency version will not install the correct version.

The option-resolver version is 3.8.x in the PHP 7.0, not 4.0.x. You can look at this log and this removed option log. It seems that this option will force to install the specific version, even the environment doesn't satisfy the requirements.

Thanks.

trowski commented 6 years ago

@peter279k Right, but we still need --ignore-platform-reqs for the nightly build. I'm considering making the composer install line conditional on the build. So something like:

  # --ignore-platform-reqs, because https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/2722
  - if [ "$TRAVIS_PHP_VERSION" == "nightly" ]; then
      composer update -n --prefer-dist --ignore-platform-reqs;
    else
      composer update -n --prefer-dist;
    fi
peter279k commented 6 years ago

@trowski, I think it's the good idea for making the different composer command conditional.

trowski commented 6 years ago

Manually merged via 2a429681bde02fb0664ea7166e0d6e4bc31e3d39. Thanks!