Conversation
froozeify
requested review from
Rom1-B and
stonebuzz
and removed request for
AdrienClairembault and
trasher
|
We should create a dedicated config file. For example If the file is not present or if his field Enabled field is optional, value is true by default. For glpi-empty, we would be able like that to set it has example with This file would should contain something like {
"enabled": true,
"lower_threshold": 50,
"upper_threshold": 75,
"fail_below_min": false
}We should probably also create a generic action for the code coverage report config in that case. (if feasible) |
froozeify
force-pushed
the
code-coverage
branch
from
50ee37c to
c59963d
Compare
froozeify
force-pushed
the
code-coverage
branch
7 times, most recently
from
435f715 to
91aa982
Compare
froozeify
force-pushed
the
code-coverage
branch
from
91aa982 to
39189e4
Compare
|
Small issue reported to me by @cedric-anne is that currently the Some plugin could have 2 "main" branch one for 10.x and one for 11.x so the coverage difference could be biased. We should see if it will have a big impact, supporting both based on PR would probably mean an even bigger ci code for check/updating those artifacts. (Probably not possible to trigger artifact refresh on target branch before running the ci and coverage on current pr, not sure github support synchronous run like that) |
|
In that case, perhaps the simplest solution would be to run it only with the main branch? So, don't display anything with 10.0./bugfixes. |
|
I've updated to run coverage only when the default branch is targeted. |
| if ! php -m | grep -q -E 'xdebug|pcov'; then | ||
| echo -e "\033[0;33mInstalling PCOV driver...\033[0m" | ||
| sudo pecl install pcov || true | ||
| fi |
There was a problem hiding this comment.
Please add pcov in the githubactions-php-apache image (inherited by githubactions-glpi-apache).
| # Explicitly load PCOV if needed | ||
| PHP_CMD="php -d extension=pcov.so" |
There was a problem hiding this comment.
I think the extention should be always loaded, but maybe with a pcov.enabled = 0 ini value by default (in the githubactions-php-apache image).
The only requirement here would the be to enable pcov through the PCOV_ENABLED env variable, see krakjoe/pcov@f7e9f7b
| file_coverage_warning_max: ${{ steps.coverage-config.outputs.file-coverage-warning-max }} | ||
| fail_on_negative_difference: ${{ steps.coverage-config.outputs.fail-on-negative-difference }} | ||
| retention_days: ${{ steps.coverage-config.outputs.retention-days }} | ||
| artifact_download_workflow_names: "Continuous integration" |
There was a problem hiding this comment.
I guess it should not be hardcoded.
| artifact_download_workflow_names: "Continuous integration" | ||
|
|
||
| - name: "Generating Markdown report" | ||
| if: github.event_name == 'pull_request' && steps.coverage-config.outputs.skip != 'true' && steps.coverage-report.outputs.file != '' |
There was a problem hiding this comment.
if: github.event_name == 'pull_request' could maybe added directly at the job level.
| ## Generate CI matrix | ||
|
|
||
| This workflow can be used to generate a matrix that contains the default PHP/SQL versions that are supported by the target GLPI version. | ||
| You can use it in combination with the `Continuous Integration` workflow, as shown in the example below. |
There was a problem hiding this comment.
| You can use it in combination with the `Continuous Integration` and the `Coverage report` workflows, as shown in the example below. |
| db-image: "${{ matrix.db-image }}" | ||
|
|
||
| coverage-report: | ||
| needs: "ci" |
There was a problem hiding this comment.
Maybe it should be trigerred only for pull requests.
| needs: "ci" | |
| needs: "ci" | |
| if: github.event_name == 'pull_request' |
| with: | ||
| plugin-key: "myplugin" | ||
| ``` | ||
|
|
There was a problem hiding this comment.
IMHO, this is not required. Indeed, we could just add a comment in the Coverage report workflow section to explain that the Continous Integration must be executed on each push on the main branch to generate an up-to-date artifact, and also to explain that a schedule of the Continous Integration is preferable to ensure that the artifact is regenerated before it expires.
4 participants
This PR add code coverage support for plugin.
By default the coverage is not enabled.
For a better output, plugin can extract the report to automatically create a comment on the PR with the report in md format.
Interconnected PR :
Whatsapp plugin got a dedicated PR to test the modification (see in the feed under for the link)
Screenshot
Once the project is merged on main you got comparison otherwise (whatsapp branch) :
Test PR with simulated code into main
https://github.com/glpi-network/whatsapp-wcoverage/pull/2
Test PR targeting not the default branch (coverage should not be run)
https://github.com/glpi-network/whatsapp-wcoverage/pull/3