Conversation
veewee
left a comment
veewee
left a comment
There was a problem hiding this comment.
Hi @Landerstraeten,
Thanks for the PR. I've added some small remarks and there are some new task checkboxes that need to be solved.
I haven't used infection myself, so it's hard for me to tell if everything works as it should.
| @@ -0,0 +1,79 @@ | |||
| # Infection | |||
|
|
|||
There was a problem hiding this comment.
Should we suggest to load infection/infection trough composer locally / globally ?
In other task documentation, this is added to the top + in the composer.json file under suggestions.
Since the documentation of infection is suggesting global installation, we might only add it in the task documentation?
There was a problem hiding this comment.
I would suggest to load it locally. I already updated the documentation.
There was a problem hiding this comment.
Can you add the composer require command to the documentation as well? e.g. https://github.com/phpro/grumphp/blob/master/doc/tasks/phpunit.md
src/Task/Infection.php
Outdated
|
|
||
| $resolver->addAllowedTypes('threads', ['null', 'int']); | ||
| $resolver->addAllowedTypes('test_framework', ['null', 'string']); | ||
| $resolver->addAllowedTypes('only_covered', ['null', 'bool']); |
There was a problem hiding this comment.
Since it's a bool, doesn't it make more sense to set the default to false and only support bool?
src/Task/Infection.php
Outdated
| $resolver->addAllowedTypes('configuration', ['null', 'string']); | ||
| $resolver->addAllowedTypes('min_msi', ['null', 'integer']); | ||
| $resolver->addAllowedTypes('min_covered_msi', ['null', 'integer']); | ||
| $resolver->addAllowedTypes('mutators', ['null', 'string']); |
There was a problem hiding this comment.
An array might be better here to make the configuration easier to read.
We can always implode the array to a comma separated string
| $arguments->addOptionalArgument('--threads=%s', $config['threads']); | ||
| $arguments->addOptionalArgument('--test-framework=%s', $config['test_framework']); | ||
| $arguments->addOptionalArgument('--only-covered', $config['only_covered']); | ||
| $arguments->addOptionalArgument('--configuration=%s', $config['configuration']); |
There was a problem hiding this comment.
This option is not documented: https://infection.github.io/guide/command-line-options.html
Does it work?
There was a problem hiding this comment.
Documentation updated
|
Mutation testing can be a very time consuming and resource intensive task, especially if it runs the entire test suite on every commit. Ideally I think this should filter to only mutate files that have changed (using the |
| return TaskResult::createSkipped($this, $context); | ||
| } | ||
|
|
||
| $arguments = $this->processBuilder->createArgumentsForCommand('infection'); |
There was a problem hiding this comment.
Infection requires xdebug or phpdbg to be installed, so I think this task should be skipped if at least one of those extensions is not installed
There was a problem hiding this comment.
I don't think grumphp is responsible to check that. If you don't have xdebug or phpdbg enabled, your tests will not run anyway.
| $arguments->addOptionalArgument('--min-covered-msi=%s', $config['min_covered_msi']); | ||
| $arguments->addOptionalCommaSeparatedArgument('--mutators=%s', $config['mutators']); | ||
|
|
||
| $process = $this->processBuilder->buildProcess($arguments); |
There was a problem hiding this comment.
Interaction should be disabled (with the -n flag)
Landerstraeten
force-pushed
the
infection-task
branch
from
d90e4bc to
2ab3e31
Compare
Landerstraeten
force-pushed
the
infection-task
branch
from
2ab3e31 to
2de3220
Compare
3 participants
New Task Checklist:
run()method readable?run()method using the configuration correctly?