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

Add notice to console output if actual msi is higher than required msi #811

Closed
wants to merge 26 commits into from
Closed

Conversation

LeoVie
Copy link
Contributor

@LeoVie LeoVie commented Oct 29, 2019

Fixes #810

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?

@maks-rafalko maks-rafalko self-requested a review October 29, 2019 19:28
@LeoVie
Copy link
Contributor Author

LeoVie commented Nov 20, 2019

I just added some unit tests for the newly added methods. I don't really get, why Travis fails now for some configurations.

@BackEndTea
Copy link
Member

Tests fail because the infection command got changed. Unfortunately we dont'have good (enough) tests for that in place, so the minimum MSI isn't hit when that class is changed.

This is a known issue though, and not considered blocking for this PR

src/Command/InfectionCommand.php Outdated Show resolved Hide resolved
src/Console/ConsoleOutput.php Outdated Show resolved Hide resolved
src/Console/ConsoleOutput.php Outdated Show resolved Hide resolved
@LeoVie
Copy link
Contributor Author

LeoVie commented Nov 22, 2019

Hm, now AppVeyor fails, too. This should also not be caused by this pr.
Seems like something is wrong with the composer executable for the end to end tests.

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.

I just checked it with my test projects, and, unfortunately, it does not work.

I run

infection --min-msi=10

Results are:

Metrics:
         Mutation Score Indicator (MSI): 100%
         Mutation Code Coverage: 100%
         Covered Code MSI: 100%

and I got the following notice:

! [NOTE] The MSI is 100% percent points over the required MSI. Consider increasing the required MSI percentage the next
!        time you run infection.

which is not true. It should be 90% over.. but not 100%.

This method returns 0 while it should return 10:

public function getMinRequiredValue(): float
{
return $this->failureType === self::MSI_FAILURE ? $this->minMsi : $this->minCoveredMsi;
}

because $this->failureType is "".

src/Exception/InvalidTypeException.php Outdated Show resolved Hide resolved
src/Exception/InvalidTypeException.php Outdated Show resolved Hide resolved
@LeoVie
Copy link
Contributor Author

LeoVie commented Nov 23, 2019

Ah, I see my fault. Will fix it soon.

@LeoVie
Copy link
Contributor Author

LeoVie commented Nov 28, 2019

This works now.

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.

Let's get the builds green and will be ready to merge. Tested - works as expected.

@theofidry
Copy link
Member

Replaced by #877

@theofidry theofidry closed this Dec 4, 2019
@LeoVie
Copy link
Contributor Author

LeoVie commented Dec 4, 2019

Oh, nice. Thank you.

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.

Notify user if msi is over specified min-msi
4 participants