New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect syntax errors during mutation analysis and differentiate them from all errors #1555
Conversation
…non-covered mutants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nitpicks, looks good!
@@ -464,6 +464,9 @@ final class ProfileList | |||
// Extensions | |||
'BCMath' => Mutator\Extensions\BCMath::class, | |||
'MBString' => Mutator\Extensions\MBString::class, | |||
|
|||
// Internal only usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal usage only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in #1557
* | ||
* @implements Mutator<Node\Expr\Variable> | ||
*/ | ||
final class SyntaxError implements Mutator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what i understand it's for testing only right? So why registering the mutator here in source code? I guess we don't have (yet) a way to register mutators only the fly for tests only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's for testing only, yes
as far as I know, we can't register mutators on the fly atm. If we remove SyntaxError
from ProfileList.php
, tests fail with "Uknown Mutator" error raised here
infection/src/Mutator/MutatorFactory.php
Lines 59 to 65 in b07e053
$knownMutatorClassNames = array_flip(ProfileList::ALL_MUTATORS); | |
foreach ($resolvedMutators as $mutatorClassName => $config) { | |
Assert::keyExists( | |
$knownMutatorClassNames, | |
$mutatorClassName, | |
sprintf('Unknown mutator "%s"', $mutatorClassName) |
|
||
yield [ | ||
<<<TXT | ||
'ParseError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case: you can now indent the heredocs: https://wiki.php.net/rfc/flexible_heredoc_nowdoc_syntaxes
I would also recommend to use nowdocs vs heredocs when possible (i.e. quote TXT
into 'TXT'
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in #1557
'coveredCodeMsi' => 75, | ||
'msi' => 50, | ||
'mutationCodeCoverage' => 83.33, | ||
'coveredCodeMsi' => 60, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is wrong. If Infection caused an arbitrary program to end with a syntax error, this was a killed mutant. Just like if mutation leads to a type error.
Implements #262
infection/abstract-testframework-adapter
before mergingFrom the first look, I didn't want to implement it since we ensure mutated code is always valid, but in the future we will allow users to write their own Mutators and use them in addition to Infection's ones, and if users' Mutator produce invalid code - users should be aware of it, because Mutators that produce invalid syntax should be fixed and should not be used since this is a wasting of time/resources.