Detect changes in ENUMs as BC breaks

[bdsl]

See https://github.com/Roave/BackwardCompatibilityCheck/issues/767

I went slightly further than discussed in the issue, reporting a BC break for cases removed, as well as for cases added. In both cases cases marked @internal do not trigger the BC break.

I think there's more to do for a complete handling of enums, including detecting if an entire Enum is removed, or becomes a trait, class or interface. Actually I see the entire Enum removed is already handled by the catch of IdentifierNotFound

https://github.com/Roave/BackwardCompatibilityCheck/blob/4c05d1a5a164ae21d5d0f6f994b116b7f71323bd/src/CompareClasses.php#L76

and I could easily extend this PR to handle became trait/interface/class by simply treating one of those as a thing that has no cases - so an enum becoming one of those is not an issue itself, its only an issue because it means cases go away. And an enum with no cases becoming a different sort of classlike is not a BC break. Let me know if you'd like me to do that.

[bdsl]

If you approve the workflow I'll try to fix any issues it detects.

I've run unit tests, psalm and phpcs on my local. It might be nice to have some end to end testing as well though. For this sort of project I wish git would work recursively, letting us have a git repo of a sample project inside this repo's test assets directory.

[Ocramius]

@bdsl done

[bdsl]

I also noticed there seems to be quite a bit of code duplication in the existing code between the handling of Classes, Traits and Interface. I think there could be a nice way to remove that duplication, but I didn't want to get into restructuring the existing code on my first PR. E.g. the ClassBased, TraitBased, InterfaceBased and new EnumBased interfaces are all the same except with differently named parameters, and the three MultipleChecksOnA... classes are also extremely similar. I didn't feel like writing MultipleChecksOnAnEnum which is partly why I implemented all the checks within one class EnumBased\ClassChanged

[bdsl]

ah didn't realise you need to approve the workflow separately for every commit. I've turned on github actions in my fork so we can see the results there: https://github.com/bdsl/BackwardCompatibilityCheck/actions

[bdsl]

The Infection check is currently failing. But on my local using vendor/bin/roave-infection-static-analysis-plugin --threads=8 --logger-html=infection.html I'm now getting 0 undetected mutants, so I'm not quite sure why. I'll have to come back to this later to see how to reproduce the way that github actions is running infection.

[bdsl]

@Ocramius can you see where I'm going wrong with the mutation tests? On my laptop (php 8.3 with xdebug in WSL) I'm getting 0 returned from the infection command as shown below. I don't know if it could be because I'm running xdebug rather than phpdbg.

$ git show --stat; git status; ./vendor/bin/roave-infection-static-analysis-plugin; echo $?
commit b5cfc9b2e141b0cd772db8e5f7ca95d9cf68a898 (HEAD -> 767-enum-cases, bdsl/767-enum-cases)
Author: Barney Laurance <barney@redmagic.org.uk>
Date:   Sun May 19 17:34:06 2024 +0100

    Roave/BackwardCompatibilityCheck#767: Remove unecassary array_values calls

    Infection detected that these were not needed

 src/DetectChanges/BCBreak/EnumBased/CasesChanged.php | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
On branch 767-enum-cases
nothing to commit, working tree clean

    ____      ____          __  _
   /  _/___  / __/__  _____/ /_(_)___  ____
   / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
 _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
/___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/

#StandWithUkraine

Infection - PHP Mutation Testing Framework version 0.27.10

Running initial test suite...

PHPUnit version: 9.6.18

  755 [============================] 7 secs

Generate mutants...

Processing source code files: 115/115
.: killed, M: escaped, U: uncovered, E: fatal error, X: syntax error, T: timed out, S: skipped, I: ignored

..................................................   ( 50 / 639)
..................................................   (100 / 639)
..................................................   (150 / 639)
..................................................   (200 / 639)
..................................................   (250 / 639)
..................................................   (300 / 639)
..................................................   (350 / 639)
..................................................   (400 / 639)
..................................................   (450 / 639)
..................................................   (500 / 639)
..................................................   (550 / 639)
....E.............................................   (600 / 639)
.......................................              (639 / 639)

639 mutations were generated:
     638 mutants were killed
       0 mutants were configured to be ignored
       0 mutants were not covered by tests
       0 covered mutants were not detected
       1 errors were encountered
       0 syntax errors were encountered
       0 time outs were encountered
       0 mutants required more time than configured

Metrics:
         Mutation Score Indicator (MSI): 100%
         Mutation Code Coverage: 100%
         Covered Code MSI: 100%

Note: to see escaped mutants run Infection with "--show-mutations" or configure file loggers.

Please note that some mutants will inevitably be harmless (i.e. false positives).
Escaped mutants:
================

Timed Out mutants:
==================

Skipped mutants:
================

Not Covered mutants:
====================
[warning] Dashboard report has not been sent: The current process is not executed in a CI build
 ! [NOTE] The MSI is 9.9% percentage points over the required MSI. Consider increasing the required MSI percentage the  
 !        next time you run Infection.

Time: 55s. Memory: 0.11GB. Threads: 1
0

[Ocramius]

Notice: You are running Infection with phpdbg enabled.

Perhaps it's using phpdbg for coverage, rather than XDebug

[bdsl]

Notice: You are running Infection with phpdbg enabled.

Perhaps it's using phpdbg for coverage, rather than XDebug

Yes possibly. I'll see if I can it working with phpdbg on my local.

[bdsl]

Spent some more time looking at this but I didn't find a way to reproduce the github actiosn behaviour on my local.

I did notice that the head on the origin branch also is showing some uncovered mutants on the action output ( https://github.com/Roave/BackwardCompatibilityCheck/actions/runs/8652266041/job/23725019284 ) although not enough to reduce the MSI below 90% and so not enough count as a build failure. But on my local with 8.2 and xdebug I get 100% MSI. (there's some crash with phpdbg )

My changes are reducing the MSI as measured in github from 92% to 88%.

I'll try and take a look again the next few days if no-one else works out what's going on.

[Ocramius]

@bdsl thanks for checking: could also be caused by underlying PHP updates, then (the rest is in composer.lock).

I'd suggest that we do a round of reviews, and then consider whether we should lower MT requirements for this to go through, as it's possibly well tested already :-)

[bdsl]

@bdsl thanks for checking: could also be caused by underlying PHP updates, then (the rest is in composer.lock).

I'd suggest that we do a round of reviews, and then consider whether we should lower MT requirements for this to go through, as it's possibly well tested already :-)

@Ocramius Sounds good. No rush.

I'm also wondering if it might be an issue that needs fixing in laminas/laminas-continuous-integration-action about how it collects line coverage stats to feed into infection.

[bdsl]

Thanks for the review @Ocramius - I'll try and find time to make changes tomorrow. Treating an enum as just a type of class does make things simpler, I was just treating them separately to follow the example of the other classlikes. Happy to change that.

I might add the check for CaseBecameInternal at the same time.

Do you have any objection to me also covering the case of an enum being changed to a different classlike by simply treating any non-enum classlike as simply a thing that has no cases. So the change is a BC break iff there is at least one non-internal case in the old enum.

[bdsl]

This is ready for re-review.

[Ocramius]

All looking quite good!

I'll do a local check of specific mutants before merging :-)

[Ocramius]

Took me a bit (because I just switched my OS / main computer), but this is pristine when running with XDebug!

692 mutations were generated:
     691 mutants were killed
       0 mutants were configured to be ignored
       0 mutants were not covered by tests
       0 covered mutants were not detected
       1 errors were encountered
       0 syntax errors were encountered
       0 time outs were encountered
       0 mutants required more time than configured

Metrics:
         Mutation Score Indicator (MSI): 100%
         Mutation Code Coverage: 100%
         Covered Code MSI: 100%

Note: to see escaped mutants run Infection with "--show-mutations" or configure file loggers.

Please note that some mutants will inevitably be harmless (i.e. false positives).
Escaped mutants:
================

Timed Out mutants:
==================

Skipped mutants:
================

Not Covered mutants:
====================
[warning] Dashboard report has not been sent: The current process is not executed in a CI build
 ! [NOTE] The MSI is 9.9% percentage points over the required MSI. Consider increasing the required MSI percentage the  
 !        next time you run Infection.

I will lower the MT requirements to get this merged, meanwhile :-)

[bdsl]

Took me a bit (because I just switched my OS / main computer), but this is pristine when running with XDebug!

Yep, I ran infection on my local when I saw that it was failing on the build, so then killed all the mutants.

There are some mutations I can imagine easily that are not tested, like deleting the untested "return trait" here or equivalently replacing the if condition with false. But I think enough is tested.

[Ocramius]

Meanwhile, :ship:

Thanks @bdsl!