Made `logs.badge.branch` capable of accepting a regular expression #1538

Ocramius · 2021-07-14T14:59:36Z

By configuring logs.badge.branch with a /-delimited regular expression, we can collect
reports about a number of branches.

{
    "source": {
        "directories": ["src/"]
    },
    "logs": {
        "badge": {
            "branch": "/^release-.*$/"
        }
    }
}

Note that logs.badge.matchBranchRegex, initially proposed as an alternative, has also been removed.

We can safely assume that a branch value starting and ending with / is a regular expression, because
git branches starting or ending with / are not valid anyway.

Fixes #1536
Fixes #814

Related: #1137
Related: #1149

This PR:

Adds new feature ...
Covered by tests
Doc PR: Documented logs.badge.branch accepting a regular expression site#220

Ocramius · 2021-07-14T15:08:29Z

tests/phpunit/Configuration/Entry/LogsAssertions.php

@@ -65,7 +63,8 @@ private function assertLogsStateIs(
            $this->assertNull($badge);
        } else {
            $this->assertNotNull($badge);
-            $this->assertBadgeStateIs($badge, $expectedBadge->getBranch());


Removed this: it made the tests much more confusing than they were, and a simpler solution was to do object-to-object equality.

object-to-object equality is always tricky due to the loose comparison, but if anyone is interested there is https://github.com/webmozarts/strict-phpunit which solves that issue and allows to remove a lot of boilertemplate

tests/benchmark/Tracing/provide-traces-closure.php

tests/benchmark/MutationGenerator/generate-mutations-closure.php

src/Logger/BadgeLogger.php

src/Configuration/Entry/Badge.php

maks-rafalko

I'm ok with the current state of PR.

One idea to discuss though: what if we make existing (in master) config key branch more clever to understand regexes?

{
    "logs": {
        "badge": {
            "branch": "master"
        }
    }
}

^ BC support, pushes the report for the exact branch - master in this case.

{
    "logs": {
        "badge": {
            "branch": "/^foo$/"
        }
    }
}

^ new feature. Infection understands this is a regex and matches with preg_match().

In this case we will keep BC and extend the functionality of branch config key.

src/Configuration/Entry/Badge.php

Ocramius · 2021-07-14T19:59:02Z

In this case we will keep BC and extend the functionality of branch config key.

Didn't want to mess with the existing branch value, as I don't know what values users have used within it so far.

What I can certainly try to do is say that if the branch value starts with / and ends with /, and is at least 3 characters long, then I treat it as a regex: a bit weak, but could work?

maks-rafalko · 2021-07-14T20:03:47Z

What I can certainly try to do is say that if the branch value starts with / and ends with /, and is at least 3 characters long, then I treat it as a regex: a bit weak, but could work?

IMO, sounds ok :) I believe the majority of the users use it like branch: master

Also, why I like the idea of having just one branch config key - it will be similar to PHPUnit's --filter, which accepts raw values as well as regular expressions, which makes it quite flexible.

Ocramius · 2021-07-14T20:17:08Z

Ok, will go back at this and simplify the patch: possibly rewrite, since that would be simpler :D

Ocramius · 2021-07-14T20:22:47Z

Thinking further about this, git branches cannot start with / nor end with /, so we're potentially good here 👍

Ocramius · 2021-07-20T11:20:59Z

@maks-rafalko I've reworked this to hide the fact that a regex or exact match happens inside a Branch, of which I do no longer expose the configuration contents.

I'll rework the docs PR.

Small suggestion: smash docs and sources together - easier for atomic diffs ;-)

Also removed `logs.badge.matchBranchRegex` docs: the upstream patch in infection/infection#1538 also got rid of it.

Ocramius · 2021-07-20T11:26:53Z

Docs PR also updated.

maks-rafalko

👍

resources/schema.json

Ocramius · 2021-07-21T02:31:53Z

Applied suggested improvements 👍

theofidry

👍 Left some comments but I don't think they are critical

tests/phpunit/Configuration/Entry/BadgeTest.php

theofidry · 2021-07-21T06:01:48Z

tests/phpunit/Configuration/Entry/LogsAssertions.php

@@ -65,7 +63,8 @@ private function assertLogsStateIs(
            $this->assertNull($badge);
        } else {
            $this->assertNotNull($badge);
-            $this->assertBadgeStateIs($badge, $expectedBadge->getBranch());


object-to-object equality is always tricky due to the loose comparison, but if anyone is interested there is https://github.com/webmozarts/strict-phpunit which solves that issue and allows to remove a lot of boilertemplate

Ocramius · 2021-07-21T08:06:01Z

@theofidry for value objects, object-to-object equality is an acceptable trade-off, so I'll leave that one as-is.

It is true that some edge cases can be troublesome, but they mostly affect floating point arithmetic and deeply nested graphs with recursion, so not really important in here.

theofidry · 2021-07-21T08:21:44Z

@Ocramius definitely, not necessary here for this case

…rough a data provider Previously, we had a single test with multiple assertions. Ref: infection#1538 (comment)

…n reporting By configuring `logs.badge.matchBranchRegex`, it is possible to set multiple branches to report their mutation score to upstream tools: ```json { "source": { "directories": ["src/"] }, "logs": { "badge": { "matchBranchRegex": "/^release-.*$/" } } } ``` Note that `logs.badge.matchBranchRegex` is mutually exclusive with `logs.badge.branch`, which is much more restrictive: the two cannot be used together. Fixes infection#1536

By configuring `logs.badge.branch` with a `/`-delimited regular expression, we can collect reports about a number of branches. ```json { "source": { "directories": ["src/"] }, "logs": { "badge": { "branch": "/^release-.*$/" } } } ``` Note that `logs.badge.matchBranchRegex`, initially proposed as an alternative, has also been removed. We can safely assume that a `branch` value starting and ending with `/` is a regular expression, because git branches starting or ending with `/` are not valid anyway. Fixes infection#1536

…rough a data provider Previously, we had a single test with multiple assertions. Ref: infection#1538 (comment)

Ocramius · 2021-07-21T08:24:02Z

Rebased and applied @maks-rafalko and @theofidry's suggestions.

Also looking forward to have this merged, so I don't have to wait for upstream approval for CI 🤷 (thanks crypto-bros, for ruining github)

tests/phpunit/Configuration/Entry/BadgeTest.php

Ocramius · 2021-07-21T08:46:50Z

Thanks for the merge!

@theofidry FWIW, avoid squashing - I put some effort in commit messages, and 0dde91a is really butchered :-\

theofidry · 2021-07-21T09:00:09Z

FWIW, avoid squashing

We squash the commits in general: the git history of a PR is rarely tidy and it too often provides close to no value for the amount of efforts it requires to keep it tidy (and even more to try to enforce it) when IMO a link to GitHub/GitLab provides a lot more information.

That said I'll be aware to avoid to do that on yours in the future, I did not took notice of it here

Ocramius changed the title ~~New logs.badge.matchBranchRegex config to match multiple branches i…~~ New logs.badge.matchBranchRegex config to match multiple branches in reporting Jul 14, 2021

Ocramius mentioned this pull request Jul 14, 2021

Documented logs.badge.branch accepting a regular expression infection/site#220

Merged

Ocramius commented Jul 14, 2021

View reviewed changes

maks-rafalko reviewed Jul 14, 2021

View reviewed changes

src/Configuration/Entry/Badge.php Outdated Show resolved Hide resolved

maks-rafalko added the Feature label Jul 14, 2021

maks-rafalko added this to the 0.24.0 milestone Jul 14, 2021

Ocramius changed the title ~~New logs.badge.matchBranchRegex config to match multiple branches in reporting~~ Made logs.badge.branch capable of accepting a regular expression Jul 20, 2021

Ocramius added a commit to Ocramius/site that referenced this pull request Jul 20, 2021

Documented logs.badge.branch used as a regular expression

4e8d383

Also removed `logs.badge.matchBranchRegex` docs: the upstream patch in infection/infection#1538 also got rid of it.

maks-rafalko approved these changes Jul 20, 2021

View reviewed changes

resources/schema.json Show resolved Hide resolved

resources/schema.json Show resolved Hide resolved

theofidry approved these changes Jul 21, 2021

View reviewed changes

Ocramius added a commit to Ocramius/infection that referenced this pull request Jul 21, 2021

Refactored BadgeTest so that we do verify branch match semantics th…

8a37265

…rough a data provider Previously, we had a single test with multiple assertions. Ref: infection#1538 (comment)

Ocramius force-pushed the feature/#1536-badge-logger-branch-regex-match-configuration branch from 79823a0 to 8a37265 Compare July 21, 2021 08:22

Ocramius added 4 commits July 21, 2021 10:22

Documented when Badge::__construct() could throw

fc5634d

Refactored BadgeTest so that we do verify branch match semantics th…

2925fe2

…rough a data provider Previously, we had a single test with multiple assertions. Ref: infection#1538 (comment)

Ocramius force-pushed the feature/#1536-badge-logger-branch-regex-match-configuration branch from 8a37265 to 2925fe2 Compare July 21, 2021 08:22

sanmai reviewed Jul 21, 2021

View reviewed changes

tests/phpunit/Configuration/Entry/BadgeTest.php Show resolved Hide resolved

theofidry enabled auto-merge (squash) July 21, 2021 08:37

theofidry merged commit 0dde91a into infection:master Jul 21, 2021

Ocramius deleted the feature/#1536-badge-logger-branch-regex-match-configuration branch July 21, 2021 08:45

Ocramius mentioned this pull request Sep 29, 2021

Re-configure infection to push mutation score reports for all stable branches psr7-sessions/storageless#196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made `logs.badge.branch` capable of accepting a regular expression #1538

Made `logs.badge.branch` capable of accepting a regular expression #1538

Ocramius commented Jul 14, 2021 •

edited

Ocramius Jul 14, 2021

theofidry Jul 21, 2021

maks-rafalko left a comment

Ocramius commented Jul 14, 2021

maks-rafalko commented Jul 14, 2021

Ocramius commented Jul 14, 2021

Ocramius commented Jul 14, 2021 •

edited

Ocramius commented Jul 20, 2021

Ocramius commented Jul 20, 2021

maks-rafalko left a comment

Ocramius commented Jul 21, 2021

theofidry left a comment

theofidry Jul 21, 2021

Ocramius commented Jul 21, 2021

theofidry commented Jul 21, 2021

Ocramius commented Jul 21, 2021

Ocramius commented Jul 21, 2021

theofidry commented Jul 21, 2021

Made logs.badge.branch capable of accepting a regular expression #1538

Made logs.badge.branch capable of accepting a regular expression #1538

Conversation

Ocramius commented Jul 14, 2021 • edited

Ocramius Jul 14, 2021

Choose a reason for hiding this comment

theofidry Jul 21, 2021

Choose a reason for hiding this comment

maks-rafalko left a comment

Choose a reason for hiding this comment

Ocramius commented Jul 14, 2021

maks-rafalko commented Jul 14, 2021

Ocramius commented Jul 14, 2021

Ocramius commented Jul 14, 2021 • edited

Ocramius commented Jul 20, 2021

Ocramius commented Jul 20, 2021

maks-rafalko left a comment

Choose a reason for hiding this comment

Ocramius commented Jul 21, 2021

theofidry left a comment

Choose a reason for hiding this comment

theofidry Jul 21, 2021

Choose a reason for hiding this comment

Ocramius commented Jul 21, 2021

theofidry commented Jul 21, 2021

Ocramius commented Jul 21, 2021

Ocramius commented Jul 21, 2021

theofidry commented Jul 21, 2021

Made `logs.badge.branch` capable of accepting a regular expression #1538

Made `logs.badge.branch` capable of accepting a regular expression #1538

Ocramius commented Jul 14, 2021 •

edited

Ocramius commented Jul 14, 2021 •

edited