Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Oct 4, 2025

return 'TrinaryLogicMutator';
}

public function canMutate(Node $node): bool
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atm this class replaces any call to a method named yes, or no.
for now it is not scoped to the TrinaryLogic class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atm this mutator works for these classes in the phpstan-src codebase - which I think is not a bad thing

grafik

@staabm
Copy link
Contributor Author

staabm commented Oct 4, 2025

current challenge: figure out a way to kill mutants across github action jobs - I will think about that

@staabm staabm mentioned this pull request Oct 5, 2025
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is failing sporadically after introducing the random order. It's probably because of:

Type::overrideType(
'date',
DateTimeImmutableType::class,
);
(which sometimes gets executed before this test and sometimes not).

I guess the test that executes this should clean up after itself.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't see PHPStan configured as mutant killer here.

Makefile Outdated
.PHONY: infection
infection:
composer require --dev infection/infection -W --ignore-platform-req=ext-mongodb
vendor/bin/infection --logger-github=false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why turning off the logger-github?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also personally I wouldn't add this to Makefile. We'll need a more complex logic in the GHA job.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added it to makefile to easy running infection locally (to reproduce CI problems)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why turning off the logger-github?

running infection in 3 php versions likely spams the 'files changed tab' with annotations.
I think we need a separate job which accumulates the separate job results and merges them togehter so we get a single final result with less false positives (mutation job on php 8.3 might error about a mutation which can only be killed in the php 8.1 test-suite)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutation job on php 8.3 might error about a mutation which can only be killed in the php 8.1 test-suite

I had this concern as well but I talked to Maks about this and given that Infection would only mutate covered lines then this shouldn't happen. If a test ran on PHP 8.3 covers a line and we mutate that line, it should fail the test on 8.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should fail the test on 8.3.

but the very same mutation will happen on 8.1/8.2 and no test will kill the mutation (in case it is a PHP 8.3 only test)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this was really true Infection would be unusable for us. We'd have to be able to invoke each phase separately and modify the results before feeding them to the next phase. (Run tests on each PHP version, feed the data into Infection, let it mutate the sources, run tests on each PHP version again ourselves and so on.)

Copy link
Contributor Author

@staabm staabm Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could run infection on multiple php versions in parallel.. collect the mutations afterwards in a new followup github action and report only those as a problem, which were not killed in any of the previous jobs.
(as long as we do not do source transformations, the mutations should be the same on all 3 jobs)

alternatively we could run infection on a single php version only

@ondrejmirtes
Copy link
Member

Some comments posted by me disappeared. I linked the --git-diff-filter / --git-diff-base / --git-diff-lines options

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PHAR distribution is the recommended one for installing Infection. I think we could just add infection to tools in setup-php action.

@staabm
Copy link
Contributor Author

staabm commented Oct 6, 2025

Some comments posted by me disappeared. I linked the --git-diff-filter / --git-diff-base / --git-diff-lines options

yeah, I did not yet activate these options, because then there would be no results reported in this PR.

@staabm
Copy link
Contributor Author

staabm commented Oct 6, 2025

The PHAR distribution is the recommended one for installing Infection. I think we could just add infection to tools in setup-php action.

I think this only make sense in case we don't want run infection locally. the more we use GitHub Actions only stuff, the harder it is to run the very same setup on a local computer.

@staabm
Copy link
Contributor Author

staabm commented Oct 6, 2025

The test is failing sporadically after introducing the random order. It's probably because of:

I did not yet see a test, which started failling more often since random order was enabled.
which particular test do you mean?

the build on the master branch is broken long before random order with the same errors

@ondrejmirtes
Copy link
Member

I think this only make sense in case we don't want run infection locally.

Yeah, I think we don't. Given the support for many PHP versions (and that we only typically have a single PHP version running locally), I think it's unlikely we'd run Infection locally.

Also I'm thinking we'll be passing GitHub Actions-specific env variables to the CLI options anyway.

@ondrejmirtes
Copy link
Member

Why is the build green even when there are escaped mutants?

@staabm
Copy link
Contributor Author

staabm commented Oct 6, 2025

Why is the build green even when there are escaped mutants?

because we did not yet define a "min MSI", meaning there is no minimum threshold yet.
if we want the test to fail when at least a single escaped mutant occurs, we need min-msi=100

@ondrejmirtes
Copy link
Member

Yeah, I want that, alongside the Git-filtering options.

We can demonstrate the failing by adding some new change here in this PR temporarily.

@staabm
Copy link
Contributor Author

staabm commented Oct 6, 2025

Also I don't see PHPStan configured as mutant killer here.

I think, as long as we only mutate with TrinaryLogicMutator its pretty unlikely that the PHPStan killer will help us.
in addition the process is already slow and adding the PHPStan killer to the mix will make it a even slower

@ondrejmirtes
Copy link
Member

I really want it though. I don't want to write tests for things that would error in PHPStan. Also we'd have more incentives to make it faster 😊

mutation-testing:
name: "Mutation Testing"
runs-on: "ubuntu-latest"
needs: ["tests", "static-analysis"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phpunit killer requires a green test run
Phpstan killer requires a green phpstan run

@ondrejmirtes
Copy link
Member

Make sure to carry over PHPStan's result cache, probably with upload and download-artifact actions. It should speed up the build significantly.

@staabm
Copy link
Contributor Author

staabm commented Oct 7, 2025

@maks-rafalko I wonder why infection treats this PHPStan result as a killed mutant?
it reads like PHPStan did not report an error at all, so I would expect a escaped mutant.

infection.log:

Escaped mutants:
================

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

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

Killed by Test Framework mutants:
=================================

Killed by Static Analysis mutants:
==================================

1) /home/runner/work/phpstan-doctrine/phpstan-doctrine/src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php:74    [M] TrinaryLogicMutator [ID] e7038292ed4647a2b1851489639d8e24

@@ @@
         }
         // testing stuff
         $obj = (new ObjectType('Doctrine\ORM\QueryBuilder'))->isSuperTypeOf($calledOnType);
-        if ($obj->yes()) {
+        if (!$obj->no()) {
             $x = 1;
         } else {
             $x = 2;

$ '/home/runner/work/phpstan-doctrine/phpstan-doctrine/vendor/bin/phpstan' '--tmp-file=/tmp/infection/mutant.e7038292ed4647a2b1851489639d8e24.infection.php' '--instead-of=/home/runner/work/phpstan-doctrine/phpstan-doctrine/src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php' '--configuration=/tmp/infection/phpstan.e7038292ed4647a2b1851489639d8e24.infection.neon' '--error-format=json' '--no-progress' '-vv' '--fail-without-result-cache'
  {"totals":{"errors":0,"file_errors":0},"files":{},"errors":[]}
  
  Result cache not used because extension file /home/runner/work/phpstan-doctrine/phpstan-doctrine/src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php hash does not match.
  Result cache was not saved because of --tmp-file and --instead-of CLI options passed (editor mode).
  Elapsed time: 29 seconds
  Used memory: 234.5 MB


Errors mutants:
===============

Syntax Errors mutants:
======================

@maks-rafalko
Copy link

maks-rafalko commented Oct 7, 2025

@maks-rafalko I wonder why infection treats this PHPStan result as a killed mutant?

I would debug the exit code. Here is the logic how Infection determines if it's killed or escaped for PHPStan process output.

https://github.com/infection/infection/blob/466255c58cdc6dbe307012cc077b9088ff3cfaa5/src/StaticAnalysis/PHPStan/Mutant/PHPStanMutantExecutionResultFactory.php#L94

We discussed with @ondrejmirtes that checking non-zero exit code is not reliable, but AFAIK nothing has been implemented on PHPStan side to improve it, so we still check (non-)zero exit code.

@staabm
Copy link
Contributor Author

staabm commented Oct 7, 2025

local debugging reveals: we get a exit code of 2, because infection internally passes --fail-without-result-cache and the result cache is not used as the PHPStan output describes:

Result cache not used because extension file /Users/m.staab/dvl/phpstan-doctrine/src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php hash does not match.
Result cache was not saved because of --tmp-file and --instead-of CLI options passed (editor mode).

@ondrejmirtes
Copy link
Member

Yeah, that's tricky. I suppose we should have a special exit code for when the result is green but result cache was not used (so that Infection does not use it to kill mutants).

Or maybe Infection can stop passing --fail-without-result-cache altogether.

@staabm
Copy link
Contributor Author

staabm commented Oct 7, 2025

Or maybe Infection can stop passing --fail-without-result-cache altogether.

I think thats the way to go. not everyone has the tools to use a result cache in CI.

@maks-rafalko
Copy link

maks-rafalko commented Oct 7, 2025

Or maybe Infection can stop passing --fail-without-result-cache altogether.

I remember I've added it by @staabm's suggestion to avoid the cases when for some (unexpected) reason we miss the cache, so for me this option did make sense so far.

Can I ask you why do we have here

Result cache not used because extension file /src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php hash does not match.

Is it the issue that can be fixed here? Or is it expected?

@staabm
Copy link
Contributor Author

staabm commented Oct 7, 2025

Can I ask you why do we have here

Result cache not used because extension file /src/Rules/Doctrine/ORM/QueryBuilderDqlRule.php hash does not match.

Is it the issue that can be fixed here? Or is it expected?

my guess is that this problem occurs because phpstan-doctrine is a phpstan extension repository (regular non phpstan extension projects would not have this case if I got it right)

@staabm
Copy link
Contributor Author

staabm commented Oct 7, 2025

I remember I've added it by @staabm's suggestion to avoid the cases when for some (unexpected) reason we miss the cache, so for me this option did make sense so far.

ohh I remember that.. while running infection, we actually have a primed cache (caused by the initial test-run). so we don't need CI tooling for the cache to work.

@ondrejmirtes
Copy link
Member

This can happen in any project with custom rules or other extensions. When Infection actually decides to mutate an extension file, it's inevitable the result cache won't be used.

@maks-rafalko
Copy link

This can happen in any project with custom rules or other extensions. When Infection actually decides to mutate an extension file, it's inevitable the result cache won't be used.

  1. so do you say here we have a cache miss issue because Infection mutates the extension's file?
  2. does it mean we need to remove passing --fail-without-result-cache by Infection?

I'm not sure I understand all the cases to make a decision.

@ondrejmirtes
Copy link
Member

I think what we're facing here is that we want to make sure the user primed the PHPStan cache before running Infection. But it can still be invalidated in these cases.

--fail-without-result-cache can sometimes fill this purpose but it's not perfect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants