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

Disallow surviving mutants in strict mode #1017

Merged
merged 4 commits into from
Jan 17, 2023

Conversation

vengaer
Copy link
Contributor

@vengaer vengaer commented Jan 14, 2023

The --help for strict mode states that "all warning messages are treated as fatal errors". While this is indeed true for anything that goes through Diagnostics::warning, there are a few messages presented as warnings that are not written via said function. The one (and at least so far only) concrete example I have come across are warnings written in printMutant in IDEReporter.cpp which emits messages similar to

<file>:144:17: warning: Survived: Replaced > with >= [cxx_gt_to_ge]
    if(currsize > size) {
                ^

Given the description for the --strict flag, it would seem as though the above should be considered a fatal error too.

The warnings written in printMutant are currently written through fprintf. While it would be simple to change this this to use Diagnostics::warning instead, that doesn't seem to be the best approach. Seeing as all the mutants have already run, all that would achieve is cut parts of the report.

Instead of omitting useful information, this checks the number of surviving mutants at the very end of mull-runner's main. If any survived, this is logged this via Diagnostics::warning. This way, mull-runner will exit with a non-zero status if there are surviving mutants in strict mode. If not in strict mode, mull-runner always exits with status 0.

This change would, of course, alter the behavior of the --strict flag which I could see being a deal breaker. If that is the case, perhaps it'd be better to add another command line argument to achieve the intended results. Either way, I'd personally find it very useful to have a way of forcing Mull to exit with an error when there are surviving mutants.

Please let me know if you'd prefer this to be handled differently

@AlexDenisov
Copy link
Member

The warnings from IDEReporter are not the same as warnings coming from Mull itself, so it is expected that they are not affected by the strict mode.

In general, the idea of failing in case of surviving mutants makes sense and is a great addition.
Though, I'd perhaps add another CLI arg to control this behavior, and maybe even enable it by default?
I don't have a strong opinion on how the CLI arg should be called, so I'm open for suggestions.

@AlexDenisov
Copy link
Member

Although, enabling it by default would likely break many integration tests, so I guess it's better to not turn it on by default just yet - unless you also want to update all the affected tests 😄

@vengaer
Copy link
Contributor Author

vengaer commented Jan 14, 2023

The warnings from IDEReporter are not the same as warnings coming from Mull itself, so it is expected that they are not affected by the strict mode.

I see. Then it's just my getting tricked by the wording :)

Though, I'd perhaps add another CLI arg to control this behavior, and maybe even enable it by default?
I don't have a strong opinion on how the CLI arg should be called, so I'm open for suggestions.

A separate argument just as well. I'll have a look at it. If the number of failing tests is managable, I'll enable it by default. As for the naming, perhaps something along the lines of --allow-surviving (or --disallow-surviving if not enabled by default)? It's a little verbose but at least it should be obvious what it does.

@vengaer
Copy link
Contributor Author

vengaer commented Jan 14, 2023

Or, perhaps, we could simply enable the option by default but explicitly disable it for the integration tests. That would save me quite a bit of work I'd imagine

@AlexDenisov
Copy link
Member

--allow-surviving sounds great!

The release CI stage of the provisioning fails due to mull_version not
being passed as a variable to ansible. Check the environment and, if no
version is set, fall back on the latest git tag with potential -dirty
suffix
When running provisioning through vagrant, pip installs its packages to
/home/vagrant/.local which is not in PATH when the lit tests are run.
Add it manually.

While this is not required for the github workflows (and probably not
macos either), it shouldn't do any harm
@vengaer vengaer force-pushed the surviving-mutants-strict-error branch 3 times, most recently from 519ca8e to 14174b7 Compare January 15, 2023 15:17
@vengaer
Copy link
Contributor Author

vengaer commented Jan 15, 2023

As discussed, mutants surviving is now treated as an error by default but can be ignored by passing the --allow-surviving CLI argument. The lit and integration tests have been updated to accomodate the new behavior.

Also made some minor changes to the infrastructure to make the CI workflow succeed when run via vagrant. Tested with the ubuntu20.04 box.

Surviving mutants would typically indicate insufficient test coverage.
As such, it makes sense for Mull to treat it as an error and exit with a
non-zero status.

This changes the default behavior of mull-runner which up until this
point has exited with zero regardless of how many mutants survived. This
behavior can be "restored" by passing the --allow-surviving CLI argument
to mull-runner.

Since the mutation scores for both fmtlib and openssl are very low, the
--allow-surviving flag is passed when running the integration tests

This does not treat the absence of mutants (i.e. infinite mutation
score) as an error
@AlexDenisov
Copy link
Member

Thank you so much @vengaer!

@AlexDenisov AlexDenisov merged commit f4ccfb5 into mull-project:main Jan 17, 2023
@vengaer
Copy link
Contributor Author

vengaer commented Jan 18, 2023

No worries! You guys have something special here, happy to help in whatever little way I can

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.

2 participants