-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add mutation tests to pipeline #334
Add mutation tests to pipeline #334
Conversation
35eea62
to
1985cf7
Compare
Merging to |
@@ -3,17 +3,4 @@ | |||
<file src="src/ComposerRequireChecker/Cli/CheckCommand.php"> | |||
<MixedArgumentTypeCoercion occurrences="2"/> |
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.
Oh, last one left? Can we completely get rid of the baseline, perhaps?
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.
Currently I don’t know how to process all the ongoing generator stuff ^^
"src" | ||
] | ||
}, | ||
"timeout": 30, |
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.
Let's lower the timeout to 5 seconds or such: we don't have long-running integration tests anyway
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.
With default 10 seconds a lot mutations were skipped due to timeout. But I‘ll try to reduce it in the dist.
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.
Done. Many skipped tests now which reduces the MSI. They are not considered T(imed out).
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.
@maks-rafalko do you perhaps know if this is default behavior of @infection, or if it's a bug to be reported? 🤔
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.
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.
Doesn't that reduce the coverage itself?
Just added a bunch of obvious @covers
to the test classes ... result:
496 mutations were generated:
384 mutants were killed
95 mutants were not covered by tests
17 covered mutants were not detected
0 errors were encountered
0 syntax errors were encountered
0 time outs were encountered
0 mutants required more time than configured
Not very promising. Probably comes from the fact that there are a lot of integration tests in contrary to unit tests. If the coverage would come from unit tests I guess @covers
would be fine.
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.
Yes, but tests should be isolated to a certain unit :D
Mutation testing integration tests is expensive, so we need to decide how coverage leaks in there.
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.
Possible way to move forward:
- accept the 30s timeout for now
- in subsequent issue add
@covers
And@group integration|unit
- infection and coverage only on
@group unit
so that coverage/MSI does not decrease
I volunteer to write additional tests ;-)
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.
I volunteer to write additional tests ;-)
Cool, your problem now :D
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.
Cool, your problem now :D
Not in this PR maybe please? ;-)
src/ComposerRequireChecker/FileLocator/LocateComposerPackageSourceFiles.php
Outdated
Show resolved
Hide resolved
@@ -37,6 +41,7 @@ public static function getData(string $path): array | |||
throw new InvalidJson('error parsing ' . $path . ': ' . $exception->getMessage(), 0, $exception); | |||
} | |||
|
|||
/** @psalm-var ComposerData $decodedData */ |
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.
Is any meaningful assertion possible here? Trusting the JSON data to be valid seems a bit extreme.
I usually use @azjezz /psl for that
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.
I guess you mean PSL\Json\typed()
?
i can introduce the dependency if you like.
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.
IMO worth adding indeed 👍
Not sure about whether phpstan/phpstan
will be happy about it - we'll see :D
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.
doc: https://php-standard-library.github.io/#/components/json/
phpstan probably won't be happy :D especially if you are using Type\shape([...]), as that only works properly when using the psl psalm plugin ( https://github.com/php-standard-library/psalm-plugin/blob/main/src/EventHandler/Type/Shape/FunctionReturnTypeProvider.php )
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.
Yep, but dropping phpstan is no biggie either
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.
If you would like, you can merge this PR as is, and i will send a PR similar to the ones i sent for Roave/* and Laminas/* 😉
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.
@dkreuer your choice on that :D
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.
Having help in that would be great.
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.
cool, will send a PR for PSL integration this weekend 👍
use Exception; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
final class FileParseFailedTest extends TestCase |
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.
Consider generally adding /** @covers
annotations - helps with keeping executed mutation tests small :)
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.
Added.
1985cf7
to
796c406
Compare
The timeout must be raised currently to not have integration tests (which currently count to coverage) being skipped.
cd52e0e
to
97b46ea
Compare
97b46ea
to
2c6fe9c
Compare
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.
🚢
Thanks @dkreuer!
Implementation for #332