Don't ignore coverage of methods without assertions

[lcobucci]

PHPUnit was not adding the coverage of tests with @doesNotPerformAssertions or $this->expectNotToPerformAssertions(); to the report, leading to wrong coverage results.

[codecov]

Codecov Report

Merging #3348 into 7.4 will increase coverage by 0.23%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                7.4    #3348      +/-   ##
============================================
+ Coverage     82.59%   82.83%   +0.23%     
- Complexity     3533     3534       +1     
============================================
  Files           143      143              
  Lines          9331     9332       +1     
============================================
+ Hits           7707     7730      +23     
+ Misses         1624     1602      -22

Impacted Files	Coverage Δ	Complexity Δ
src/Framework/TestResult.php	75.55% <100%> (+0.06%)	169 <0> (+1)	⬆️
src/Framework/TestCase.php	76.27% <0%> (+0.12%)	304% <0%> (ø)	⬇️
src/Framework/Assert.php	95.06% <0%> (+1.23%)	236% <0%> (ø)	⬇️
src/Framework/Constraint/LogicalXor.php	66.66% <0%> (+30.55%)	14% <0%> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bab8bbd...612ea7c. Read the comment docs.

[sebastianbergmann]

I was able to agree to adding @doesNotPerformAssertions and $this->expectNotToPerformAssertions();. I am not able to agree to marking code as covered that is not covered by an assertion.

Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it.

[lcobucci]

@sebastianbergmann so do you expect people to rely on $this->addToAssertionCount() to test the happy path of methods that raise exceptions? Like:

public function assert(): void
{
    if (/** condition */) {
        throw new Exception();
    }
} 

I respect your decision but I find it hard to agree that setting the test to not expect an assertion is not an assertion, since it actually sets the intention of the test. I would even argue that it is similar to setting the test to expect an exception - IMO this mindset plays nicely with @localheinz's contribution to fail the test if any assertion happened.

[lcobucci]

It's important to mention that I don't really care about having 100% of coverage but the coverage affects my mutation tests and msi and covered msi are relevant to me.

[lcobucci]

@sebastianbergmann the issue #2484 and all related PRs/issues shows how people are using @doesNotPerformAssertions...

[sebastianbergmann]

@lcobucci Sounds like a documentation issue to me.

[lcobucci]

@sebastianbergmann alright, I can send a PR to the doc repo to solve that issue.

Could you please confirm that the expected ways to assert that no exception has been thrown are: using $this->addToAssertionCount() or adding a dummy assertion after the method that is expected to not raise the exception?

Even though I might not agree with either it's important to make things clear on the docs 👍