Skip to content
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

Allow the initial test suite to be skipped #1042

Merged
merged 3 commits into from Feb 26, 2020

Conversation

duncan3dc
Copy link
Contributor

@duncan3dc duncan3dc commented Feb 11, 2020

This PR:

In an effort to reduce our build times we started to use the --coverage option to use previously generated code coverage stats, however we noticed that Infection still runs the entire test suite before beginning mutations, effectively doubling our build time.

I searched through the docs and the only thing I could find that explained this is here:
"This in no way restricts the initial Infection check on the overall test suite which is still executed in full to ensure all tests are passing correctly before proceeding."

If Infection doesn't need any other information from this initial run other than the coverage, then it should be possible to bypass it, which is what this PR allows.

I'll happily add tests and a docs PR, but I didn't want to do too much work in case there was something I'd missed that means this isn't a practical option

Copy link

@tests-checker tests-checker bot left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to Infection. We noticed you didn't add any tests. Could you please add them to make sure everything works as expected?

@sanmai
Copy link
Member

sanmai commented Feb 12, 2020

This is a very dangerous proposition because if tests are not in the passing state where you think they are, even valid mutations will be looking like they were caught.

@maks-rafalko
Copy link
Member

maks-rafalko commented Feb 12, 2020

This is a very dangerous proposition because if tests are not in the passing state where you think they are, even valid mutations will be looking like they were caught.

That's true. Even if your tests pass from original PHPUnit run, they can fail when being executed by Infection since this is subprocess, different config, etc.

At the same time, I should admit that I needed this option many times, and had to comment out initial tests run right in the code (though, this was during developing).

What if we add this option with a big red warning in terminal, explaining all the consequences?

Also, this option can be allowed to use only with provided coverage (--coverage). Otherwise, it does not make any sense.

@sanmai
Copy link
Member

sanmai commented Feb 12, 2020

I think a warning is a must.

@duncan3dc
Copy link
Contributor Author

I will carry on with this when I get some time then, fix the unit tests and add an e2e test for this functionality. Do you want me to prepare a docs PR too, or would you rather decide on the wording yourselves?

@maks-rafalko
Copy link
Member

Please go ahead with the docs as well 👍 Thank you

duncan3dc added a commit to duncan3dc/site that referenced this pull request Feb 14, 2020
@duncan3dc duncan3dc force-pushed the skip-initial-test-suite branch 2 times, most recently from d4830a5 to 6c12e23 Compare February 14, 2020 14:14
Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Looks very nice. A couple of small comments

src/Engine.php Outdated Show resolved Hide resolved
src/Engine.php Outdated Show resolved Hide resolved
Copy link
Member

@maks-rafalko maks-rafalko left a comment

Choose a reason for hiding this comment

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

Please run make cs and we are good to merge

@duncan3dc duncan3dc force-pushed the skip-initial-test-suite branch 2 times, most recently from 5e7af45 to 916c574 Compare February 18, 2020 16:00
@duncan3dc
Copy link
Contributor Author

make cs looks good locally, although PrettyCI seems very upset

@theofidry
Copy link
Member

Might be cache related: try removing the PHP-CS-Fixer cache and running it again

src/Command/InfectionCommand.php Outdated Show resolved Hide resolved
src/Command/InfectionCommand.php Outdated Show resolved Hide resolved
src/Console/ConsoleOutput.php Outdated Show resolved Hide resolved
@duncan3dc duncan3dc force-pushed the skip-initial-test-suite branch 2 times, most recently from 0fe1188 to fc45487 Compare February 24, 2020 12:00
@sanmai
Copy link
Member

sanmai commented Feb 26, 2020

Would you mind checking that "Allow edits from maintainers" checkbox?

Else please update the branch. Thanks.

@duncan3dc
Copy link
Contributor Author

I've done both 👍

@maks-rafalko
Copy link
Member

maks-rafalko commented Feb 26, 2020

Could you look into failing tests please? I will merge it as soon as it's green

@duncan3dc
Copy link
Contributor Author

I think we're all good now, thanks for bearing with me

@maks-rafalko
Copy link
Member

This is a great contribution, thank you @duncan3dc!

@maks-rafalko maks-rafalko merged commit aa8a886 into infection:master Feb 26, 2020
maks-rafalko added a commit to infection/site that referenced this pull request Feb 26, 2020
* New option to skip the initial test suite

Doc update for infection/infection#1042

* Style the warning message

Co-Authored-By: Maks Rafalko <b0rn@list.ru>

Co-authored-by: Maks Rafalko <b0rn@list.ru>
@duncan3dc duncan3dc deleted the skip-initial-test-suite branch February 26, 2020 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants