-
Notifications
You must be signed in to change notification settings - Fork 125
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
Add support for Github Actions #275
Add support for Github Actions #275
Conversation
Smolevich
commented
Oct 21, 2019
- Add support Github actions for collector
- Add test case
@@ -64,6 +64,7 @@ public function collect(array $env) | |||
->fillAppVeyor() | |||
->fillJenkins() | |||
->fillLocal() | |||
->fillGithubActiions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: double i
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Travis build for PHP with version 7.1 failed
I don't know that it means |
Do we need Github Actions ci file for this PR? |
@keradus, can you review please? |
Could the maintainers review / merge this please. |
As I understand this repo has one member - @keradus and judging by the status on the github he is on vacation |
Thanks @Smolevich. |
…ons (see #963) Description ----------- Unfortunately, we have to delay using GitHub actions to generate the coverage reports until php-coveralls/php-coveralls#275 has been addressed. Commits ------- 88c0c24 Remove the coverage workflow until php-coveralls supports GitHub actions
Unfortunately, this does not work for pull requests, because |
hey. exactly, i was on hols. thanks for understanding. I took care of @Smolevich, can you please look at @leofeyer concern? thanks! |
Ok, i see case @leofeyer. @keradus, What branch should merge in branch for PR? |
cb09f1c
to
42b8068
Compare
@keradus can you review please? |
I've tried to test this PR again (instructions here) but it's not working, I believe it's because coveralls doesn't yet handle the "github" service_name. When I modify this PR branch to hardcode
this seems to work ☝️ However the PR in its current state creates a coveralls.json like this:
which does not work 😞 It fails with the following output:
The official coveralls docs suggest that the I suspect it's failing because coveralls doesn't yet recognise the "github" Should we set the service name and job ID to be null or not present for the moment until Coveralls handles them correctly? |
This works well for me simPod@b9c9d4c I reversed engineered their js implementation and api so I'm setting the same fields as they do. |
@emmetog can i show your test? |
@emmetog you can see on work PR here https://github.com/jenssegers/laravel-mongodb/pull/1920/checks |
@Smolevich For my test I first set the env vars:
Then I run phpunit to generate the clover coverage xml and run coveralls to push them:
I'm using a private GitHub repo. The php-coveralls package in my vendor/ is using this exact PR which can be set up by following the instructions here. @simPod I tried removing the branch as you suggested but still no joy 😞 Here's the
Could you add a step in your github-actions file to show the generated - name: Debug converalls.json
run: cat /home/runner/work/laravel-mongodb/laravel-mongodb/build/logs/coveralls-upload.json |
What is the version php-coveralls in composer.json? |
Interestingly, setting - name: Upload coverage results to Coveralls
run: vendor/bin/php-coveralls --coverage_clover coverage/clover.xml --json_path coveralls.json -vvv
env:
COVERALLS_RUN_LOCALLY: 1 From what I've seen this also sets the branch and pull request link details correctly in the coveralls UI. This suggests that the changes which are made when if (isset($this->env['COVERALLS_RUN_LOCALLY']) && $this->env['COVERALLS_RUN_LOCALLY']) {
$this->env['CI_JOB_ID'] = null;
$this->env['CI_NAME'] = 'php-coveralls';
$this->env['COVERALLS_EVENT_TYPE'] = 'manual';
// .... |
See https://github.com/Smolevich/reindexer-client/runs/431760052?check_suite_focus=true |
@@ -74,9 +74,9 @@ protected function collectBranch() | |||
|
|||
foreach ($branchesResult as $result) { | |||
if (strpos($result, '* ') === 0) { | |||
$exploded = explode('* ', $result, 2); | |||
preg_match('~^\* (?:\(HEAD detached at )?([\w/]+)\)?~', $result, $matches); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks for branches which have hyphens (-
) and other non-word characters in them. This method incorrectly extracts my
for the branch name when the full branch name is my-branch
for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Smolevich I'll fix this and send you PR, k?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My branch has hyphen and this code don't break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(edit: fixed the code of the test)
Here's a test which replicates the problem (feel free to use it in this PR):
/**
* @test
*/
public function shouldParseCaseInsensitive()
{
$branches = [
' TEST-123',
'* TEST-456',
' TEST-789',
];
$gitCommand = $this->createGitCommandStubWith($branches, $this->getHeadCommitValue, $this->getRemotesValue);
$object = new GitInfoCollector($gitCommand);
$result =$object->collect();
$this->assertEquals('TEST-456', $result->getBranch());
}
I've seen that github actions checks out the code in detached HEAD (HEAD detached at...)
but when running locally it's not detached and it returns TEST
instead of TEST-456
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also broke prod release :(
https://travis-ci.org/github/php-coveralls/php-coveralls/jobs/733117026#L565
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also recovered support for hyphens
#298
Hello, I have merged the PR into my forked repo, added parallel job support, and published a new release, feel free to use it. https://github.com/twinh/php-coveralls#github-actions Add a new step after phpunit generate coverage report. - name: Upload coverage results to Coveralls
env:
COVERALLS_REPO_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
composer global require twinh/php-coveralls
php-coveralls --coverage_clover=build/logs/clover.xml -v |
$jobId = $githubRef; | ||
} | ||
|
||
$this->env['CI_JOB_ID'] = $jobId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$this->env['CI_JOB_ID'] = $jobId; | |
$this->env['CI_JOB_ID'] = $this->env['GITHUB_RUN_NUMBER']; |
$this->env['GITHUB_RUN_NUMBER']
is now available should be used instead of the custom commit hash based value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OndraM maybe, but seems like maintainers ignore this PR and communication with community is broken
@OndraM @keradus Could we somehow support GHA? Maybe with #296? |
Anyone considering / already migrated to codecov and can share experience? |
Coveralls is the superior service. |
they've got a github action for it, works fine. |
They use the GH annotations API so that you get comments on the lines without code coverage on the pull request page itself. |
I merged the extended version of this PR - the #296. big thanks to work on that! |