Skip to content

Conversation

localheinz
Copy link
Contributor

@localheinz localheinz commented Feb 14, 2020

This PR

  • requires PHP 7.2 in generator/composer.json
  • requires phpunit/phpunit:^8.5.2
  • sets up PHP with the pcov extension enabled
  • enables step related to archiving and reporting code coverage

Follows #191 (comment).

💁‍♂ The update to PHP 7.2 allows us to install phpunit/phpunit:^8, which has support for collecting code coverage with pcov, which in turn is a lot faster than using Xdebug.

@localheinz localheinz requested a review from Kharhamel February 14, 2020 18:52
@localheinz localheinz marked this pull request as ready for review February 14, 2020 18:52
@localheinz
Copy link
Contributor Author

@Kharhamel

The checkout with svn appears to be a bit flaky.

Perhaps it makes sense to switch to using the mirror at https://github.com/php/doc-en instead?

@localheinz
Copy link
Contributor Author

@Kharhamel

Needs some work on the tests - will fix after dinner!

run: "php vendor/bin/php-coveralls -v"
working-directory: "generator"
env:
COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_REPO_TOKEN }}
Copy link
Contributor Author

@localheinz localheinz Feb 15, 2020

Choose a reason for hiding this comment

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

The secret COVERALLS_REPO_TOKEN needs to be provided.

However, secrets will not be available when actions are triggered by actors who do not have write privileges. That's why I added continue-on-error here.

For reference, see:

Copy link
Collaborator

Choose a reason for hiding this comment

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

That means that anyone other than moufmouf or me will not benefit from the code coverage report.
That makes it kinda useless. Could we not use the option --dry-run to do the report locally and make it create an error if the code coverage goes down?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@moufmouf
Copy link
Member

Hey @localheinz ,

Thanks a lot for this PR. I think we will put the COVERALLS_REPO_TOKEN environment variable in clear directly in the repo (so that we get code coverage from the forks too). It's never good to put a secret publicly, but that's "only" a secret for Coveralls.

@moufmouf moufmouf merged commit dd1e053 into thecodingmachine:master Feb 24, 2020
@localheinz localheinz deleted the fix/coverage branch February 24, 2020 11:33
@localheinz
Copy link
Contributor Author

Thank you, @Kharhamel and @moufmouf!

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.

3 participants