Add github logger to be able to use Annotations on GitHub Actions

[maks-rafalko]

This PR:

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

Fixes #1322

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]

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

[sanmai]

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.

[sanmai]

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

[sanmai]

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.

[sanmai]

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

[maks-rafalko]

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

what is it?

[sanmai]

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

[sanmai]

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]

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]

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]

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.

[maks-rafalko]

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

my opinion:

1. 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.

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.

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]

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]

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]

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

1. how would we do it from PHP code? (filter out changed/added files only)
2. 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]

1. shell_exec('git diff origin/$GITHUB_BASE_REF --diff-filter=AM --name-only') and take it from there.
2. 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]

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

[maks-rafalko]

Thank you all for the great review and ideas!

[sanmai]

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]

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