Tests without assertions does not generate coverage

[edruid]

Q	A
PHPUnit version	6.5.6
PHP version	7.0.27
Installation Method	Composer

I have a test that does not do an assertion (it tests a guard that throws an exception if it fails). This test doesn't provide coverage for the code it runs. If I add $this->assertTrue(true); at the end of the test coverage is provided.

/**
 * @doesNotPerformAssertions
 */
public function testVerifyActivityAccessSameUserAllowed()
{
    $user = $this->userFactory->create();

    app(UserVerifier::class)->verifyActivityAccess($user, $user);
}

phpunit is run like

vendor/bin/phpunit --coverage-html cov path/to/test.php

composer_info.txt

[sebastianbergmann]

This is expected behaviour.

A test that has no expectations and no assertions is considered by PHPUnit as risky (by default, since PHPUnit 6). PHPUnit does not generate code coverage for risky tests.

[edruid]

The documentation for @doesNotPerformAssertions states that the test is no longer considered risky. Should it not then also generate coverage?

[Stadly]

I agree that it makes sense that @doesNotPerformAssertions would lead to code coverage generation.

[simPod]

Is there a way to mark it as a non risky and therefore make coverage count?

[edruid]

Is there a way to mark it as a non risky and therefore make coverage count?

Well what I resorted to is the idiotic $this->assertTrue(true); at the end of the test...

[simPod]

Same here but that's a bit dumb and @doesNotPerformAssertions has no point then :/

[koriym]

Same here with $this->expectNotToPerformAssertions();

[nibynool]

Wasted an entire morning trying to work out why I wasn't getting coverage. Turns out this is the exact issue.

Would love for this to be re-opened as it does not seem intuitive that a test marked as @doesNotPerformAssertions (ie non-risky) is excluded from coverage.

Alternatively, some additional input from @sebastianbergmann to suggest a valid workaround or to change my mind about this would also be good :)

[sebastianbergmann]

The purpose of $this->expectNotToPerformAssertions(); (and @doesNotPerformAssertions) is to suppress the risky test warning. It is my opinion that a test that does not perform assertions should not result in code being marked as being tested (covered by a test).

[simPod]

I think the reason why assertTrue(true) is usually done is to assert that the tested code does not throw an exception. See issue question here or this test for example.

Something contrary to expectException() should solve all valid use cases (correct me if I'm wrong, I tried to gather all valid examples of assertTrue(true)). I'm thinking of something like expectExceptionNotThrown().

[andreasschroth]

Well I also came here at first to suggest the issue being reopened, but I have to agree with @sebastianbergmann . I think this only happens when a test isn't properly written.

A test I saw the problem in code coverage:

if (!\is_array($config)) {
    self::fail('The config needs to be an array.');
}

Obviously this test is not properly written and should be rewritten to:

self::assertIsArray($config);

[edruid]

@sebastianbergmann If I have a validator that only consists of a throw statement within an if-statement, how should one test - and get coverage - for the positive case? For example, how should I test the positive case for this code?

function verifyPositive(int $number) {
    if ($number <= 0) {
        throw new Exception('Number is not positive');
    }
}

[sebastianbergmann]

This is neither a discussion nor a support forum, sorry.

[simPod]

@andreasschroth you're right but that is completely different case

[andreasschroth]

@simPod Yep, I know. Just wanted to share some piece of code I stumbled upon which was clearly poorly written.

@edruid Imo, the question here is - does the positive case even need to be tested in this case? How can you assert something that is non-existent? If the number is positive, nothing is done / executed / thrown / happens. As Sebastian doesn't seem to want this to be discussed here, feel free to send me an e-mail (see profile) if you want to discuss it further.

[fullbl]

IMHO it's possible that a class is being tested without assertions, for example a factory.
If I try to assert that the result of a factory is an instance of a specific class, psalm will throw an error: why are you checking that an already known class (hinted from the return type) is of that type?
So, I can choose to suppress psalm, or I can choose to test something random, just for having the test go green!

[JelleSolvari]

@sebastianbergmann If I have a validator that only consists of a throw statement within an if-statement, how should one test - and get coverage - for the positive case? For example, how should I test the positive case for this code?

function verifyPositive(int $number) {
    if ($number <= 0) {
        throw new Exception('Number is not positive');
    }
}

I have several classes which only contains methods like this, would love to see my coverage turn green.

Please reopen en reconsider <3

[mabar]

For anyone wondering how to solve the problem: My general solution is to wrap code in try-catch and expect the exception to be null. I am not sure about psalm, but most strict settings of phpstan does not complain.

$exception = null;
try {
	$authorizer->allow('role', 'unknown');
	$authorizer->deny('role', 'unknown');
} catch (Throwable $exception) {
	// Handled below
}

self::assertNull($exception);

[TheKeymaster]

@sebastianbergmann

The purpose of $this->expectNotToPerformAssertions(); (and @doesNotPerformAssertions) is to suppress the risky test warning. It is my opinion that a test that does not perform assertions should not result in code being marked as being tested (covered by a test).

That makes sense, but what if the assertions that are being made afterwards would not even effect the covered code? You're saying that, as soon as a test doesn't have any assertions, the coverage that this test would cover is no longer valid. However, as soon as any assertion is made, which may not even relate to the code coverage, it is valid again.

I remember in older versions of PHPUnit that if a mock made an assertion by explicitly ensuring that a method of a mock has been called with certain arguments, this would also be a valid assertion.
This did change however in some version of PHPUnit, which makes it impossible to create a test that only ensures that one or more mock methods are called with specific arguments, without having an assertion at the end. Because as soon as I would add $this->expectNotToPerformAssertions() all my code coverage would be gone.

The only reasonable solutions for this is to either restructure my code, or add an assertion that doesn't assert anything. Restructuring the code is easier said then done, as I may have many dependents on my code, so I can not just change it and hope no one will complain.

I would love a way to tell PHPUnit that my mocks already cover the code that I want to test, like in previous versions. I do not know the details of why it was changed, or even if it was changed on purpose, or just a side effect of another change.

Here is an example of a custom Client, where I want to ensure the Client sends a request with a proper URL.

class ClientTest extends TestCase
{
    public function testClientBuildsRequestUrlBasedOnSetProtocol(): void
    {
        // Third party Client e.g. Guzzle.
        $guzzleClient = $this->getMockBuilder(GuzzleClient::class)
            ->disableOriginalConstructor()
            ->onlyMethods(['get'])
            ->getMock();

        $client = new Client($guzzleClient);
        $client->setProtocol('https://'); // Some random setting that we want to test against. 

        $guzzleClient->expects($this->once())
             ->method('get')
             ->with(['https://some-url.com']);

        $client->getDataFromSomeUrl();

        // Now I either check the return value of my client, which isn't the purpose of the test,
        // or I mark this test as having no assertions, which leads to all my code coverage being lost,
        // or simply add an assertTrue(true), which also isn't the best option.
        // Something like an assertMockArguments (this is a really bad name IMHO) could solve this issue.
        $this->assertMockArguments();
    }
}

[uuf6429]

@sebastianbergmann just wanted to throw in my two cents on this, after almost opening a new bug report.

Long story: last week I wrote a test that uses an anonymous class. It worked fine, but coverage didn't work. Did some searching and couldn't find why. I started preparing a small POC to add to a bug report, but I couldn't reproduce the behaviour.
Today I decided to give it another look and I still couldn't reproduce.
Eventually, I realised that the problem was not the anon class, but expectNotToPerformAssertions.

During this, I saw another interesting behaviour. I had such a test with expectNotToPerformAssertions and by mistake I made it assert something in the end. The test failed with a "This test is annotated with "@doesNotPerformAssertions" but performed 1 assertions".
Makes perfect sense (except the annotation part, I actually called the method - but no biggie).

BUT shouldn't this behaviour also apply to coverage? Like, in my original problem, shouldn't it have failed with "no assertions expected, but code was covered"?

I get your point, but due to lack of documentation (*read on) and the not very obvious behaviour, I suspect more bug reports will crop up on this subject.

On the point of documentation, aren't expectXXX also assertions? (at least, that's what the error message from before implied) If so, shouldn't they be listed in the documentation appendix as well? Also, as mentioned before, doesnotperformassertions doesn't mention expectNotToPerformAssertions() nor anything about coverage.

(I've been trying to update the documentation since a few hours ago, but this sphinx/rst/whatever stuff isn't easy..)
edit: I give up .. whoever decided to use such a messy 'magical' system for content that is 95% static, can go and fix the documentation. By the way, the "Edit on GitHub" doc links are broken..