Add github logger to be able to use Annotations on GitHub Actions by maks-rafalko · Pull Request #1368 · infection/infection

maks-rafalko · 2020-10-22T21:54:10Z

This PR:

Covered by tests
Doc PR: Document --git-diff-filter, --git-diff-base, --logger-github tions site#183

Description

This feature allows adding errors/warnings for particular commits/PRs - GitHub Annotations

Basically, this feature adds escaped mutants information (diff) to particular lines where modified source code wasn't detected by tests:

Things to note

Unfortunately, GitHub Annotations does not allow using syntax highlighting inside annotations, so we can't display a pretty diff, as well as using <details> tag to be able to show/hide huge text.
Currently, I've added a separate GH Action to run Infection just for the added files in order to display warnings.
Displayed warnings does not fail the build

Goal

This will greatly improve review process as it will be immediately clear what parts of the code are not covered by tests properly (thus escaped mutants)

Suggestions are appreciated.

sanmai · 2020-10-22T22:11:57Z

Just a quick note that there’s hardly any reason why Infection can’t output GitHub compatible annotations straight away.

sanmai · 2020-10-22T22:14:22Z

I mean we can skip this XML and cs2pr altogether by outputting the annotations right from Infection. This will also solve the problem I’ll mention in the review comments in a moment.

.github/github-functions.sh

sanmai · 2020-10-22T22:27:49Z

infection.json.dist

Just like it so it could be:

"github": "/dev/stdout" // or /dev/stderr, whichever works best

And the it could output the annotations directly:
https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions#setting-a-warning-message

If you look in the source of cs2pr, these annotations are pretty damn simple:

https://github.com/staabm/annotate-pull-request-from-checkstyle/blob/79e8cb48fb729d31684d02fe856dbf2282b65d6f/cs2pr#L108-L137

It is not to say we should scrap this new checkstyle report, but it won't exactly work here.

I think it can be even better with "github" progress reporter.

I think it can be even better with "github" progress reporter.

what is it?

Like ProgressFormatter or DotFormatter. Set with --formatter option.

.github/workflows/mutation-testing-annotations.yaml

sanmai · 2020-10-22T23:35:36Z

If I understood correctly, PHPStan enables this reporter automatically (?) on GitHub CI.

Their implementation: https://github.com/phpstan/phpstan-src/blob/c99b7f8c09fe6e319624ef3b0b66f43d30056fe4/src/Command/ErrorFormatter/GithubErrorFormatter.php

maks-rafalko · 2020-10-22T23:42:37Z

If I understood correctly, PHPStan enables this reporter automatically (?) on GitHub CI.

seems so, but I'm not sure it's a good choice TBH for Infection. Without proper filtering (as we do it right now), this will overload the diff with thousands of warnings by default (which MT can produce with even not very big projects)

sanmai · 2020-10-23T00:19:10Z

Without proper filtering (as we do it right now), this will overload the diff with thousands of warnings by default

If Infection can detect it is running on GitHub (provided it was asked to do that), then it can fetch and run against changed files just as well.

sanmai · 2020-10-23T00:20:00Z

For example, specifying --formatter=github might trigger all this new machinery with changed files etc.

I agree it's not the best idea to enable this behavior by default.

.github/workflows/mutation-testing-annotations.yaml

maks-rafalko · 2020-10-23T09:07:17Z

For example, specifying --formatter=github might trigger all this new machinery with changed files etc.

my opinion:

formatter and logger - two different concepts. Formatter is for displaying the MT process, during the run (on each step). Logger - doing smth with the calculated results after MT is done. So here we are dealing with logger. We don't want to display GH Annotation on each step.
I don't like an idea of making Infection too opionated here. Why should it decide for developer what files to filter? Why only Added files but not Modified? Why not both? Why both? And so on.

So, the current state of PR IMO is ideal - we give a developer new logger that can display GH Annotations to a file / stdout. Developer decides what files to mutate in order to use it - all the files in the project, just added new files, just modified or a combination.

What I think we should add (but in a separate PR), is an ability to set logger by command line option, like --logger-checkstyle=php://stdout, or any other of supported loggers

Why? To be able to not use checkstyle report for all the Infection runs in all currently available GH Actions, but to specify it only for Infection / Mutation Testing Code Review Annotations one

sanmai · 2020-10-23T09:08:58Z

Why? To be able to not use checkstyle report for all the Infection runs in all currently available GH Actions, but to specify it only for Infection / Mutation Testing Code Review Annotations one

That's what I was thinking when I proposed --formatter=github. This will probably work just as well.

sanmai · 2020-10-23T09:16:02Z

2. I don't like an idea of making Infection too opionated here. Why should it decide for developer what files to filter? Why only Added files but not Modified? Why not both? Why both? And so on.

Infection is already very opinionated in a similar way. For example, by choosing to add a whitelist (or what's it called now) to phpunit.xml if there is none. Making Infection collect all the necessary details for a user if the run it with --formatter=github or --logger-github is a step in the same direction. A user does not need to know or care what's under the hood. Infection just works.

Imagine you're writing a blog post. What would be better to write, that Infection will now automatically mutate only changed files if run with --logger-github, or that Infection can do that only if you're brave enough to sprinkle it with a bunch of hardly readable git/paste magic?

(I'd take it if you say that Infection shouldn't add <whitelist> if there's none, that's a major blocker for our issue with PHPUnit 9.3.)

maks-rafalko · 2020-10-23T09:21:34Z

What would be better to write, that Infection will now automatically mutate only changed files if run with --logger-github

how would we do it from PHP code? (filter out changed/added files only)
I still don't like the idea to decide what files to leave after filtering: Added, Modified, both. How to decide? Here in Infection we removed Modified because of too many failed builds due to the low MSI in the old files. But why should we do the same for all the projects? Then, we need to add settings to enable / disable M files? Doesn't sound good

Imagine you're writing a blog post. What would be better to write, that Infection will now automatically mutate only changed files if run with --logger-github, or that Infection can do that only if you're brave enough to sprinkle it with a bunch of hardly readable git/paste magic?

I'm ok with a couple of lines to be added to CI config. This tool is for developers

sanmai · 2020-10-23T09:27:31Z

shell_exec('git diff origin/$GITHUB_BASE_REF --diff-filter=AM --name-only') and take it from there.
It can be --logger-github=AM with a default set to A

If there's --logger-github and isn't GITHUB_BASE_REF, then it's an error (or a pass?).

maks-rafalko · 2020-10-23T09:31:10Z

Sounds good. I will give it a try, thanks!

.github/workflows/mutation-testing-annotations.yaml

…to `github` Automatically mutate only changed/added files when `--logger-github` is used

… to `--logger-github`

…ix escaped mutant

maks-rafalko · 2020-10-31T17:10:33Z

Thank you all for the great review and ideas!

sanmai · 2020-11-27T20:30:36Z

There's a Symfony's special reporting component now:

https://github.com/symfony/symfony/blob/f0aa4e7df17d40eadf540c6c3d0f9e6feac67648/src/Symfony/Component/Console/CI/GithubActionReporter.php#L21

Slamdunk · 2022-06-07T12:25:44Z

src/Logger/GitHub/GitDiffFileProvider.php

+    {
+        return (string) shell_exec(
+            sprintf(
+                'git diff %s --diff-filter=%s --name-only | grep src/ | paste -sd ","',


Hardcoded src/ is likely to be wrong, it should be picked by config.
PR coming soon

## Context The user may apply a git filter in two different ways: - `--git-diff-filter=AD` - `--git-diff-lines` which is an alias for `--git-diff-filter=AM`. Now the issue is that since this is two distinct options, we need to distinguish the two inputs and validate them accordingly as they are mutually exclusive. I think there was some confusions before. The code evolved a lot since the filters were first introduced (#1368, #1632, #2394, #2542). Checking that the right options are passed correctly is now clearly enforced in `RunCommand` (see #2542) and `ConfigurationFactory` responsibility is to pass that `$sourceFilesFilter: string` value and the git related values when a git filter is applied. ## Problem Right now `Configuration#isForGitDiff` is set only if the user passed `--git-diff-lines`. This is incorrect as the goal of this value is to convey that we are applying a git diff of sorts for: - `IndexXmlCoverageParser` to throw a more understandable exception to the user when no coverage is found. - `FileMutationGenerator/NodeMutationGenerator` to only mutate the nodes that are part of the diff. In other words, it should not matter which option was used. ## Solution This PR corrects the value.

maks-rafalko added the Feature label Oct 22, 2020

maks-rafalko added this to the 0.19 milestone Oct 22, 2020

maks-rafalko requested review from sanmai and sidz October 22, 2020 21:54

maks-rafalko marked this pull request as draft October 22, 2020 22:06

sanmai reviewed Oct 22, 2020

View reviewed changes

.github/github-functions.sh Outdated Show resolved Hide resolved

sanmai reviewed Oct 22, 2020

View reviewed changes