Pest: Remove Warnings for no assertions

[miscbits]

In line with other testing frameworks, a test that has no function should not throw warnings when there are no assertions. Example of this (using a fake service)

<?php

use App\Services\CalculatorService;


// This should be a valid state
it('can subtract');

// This is fine to throw a warning or an error
it('can add', function() {
        $calculator = new CalculatorService();
    $calculator->add(1,2);
});

Currently both of these throws warnings. I suggest the first should just show yellow highlighted text with no warning and the second should throw a warning.

[miscbits]

Another example to make this clearer. The following test suite should return the following results

<?php

use App\Services\FundService;
use App\User;

beforeEach(function() {
    $this->user = factory(User::class)->make();
    $this->user->funds = 10.5;
    $this->fundService = new FundService();
});

test('user has funds', function() {
    // this should return false
    $this->fundService->hasFunds($this->user, 20);
    // this shoudl return true
    $this->fundService->hasFunds($this->user, 20);
});

it('can subtract funds', function() {
    $this->fundService->subtractFunds($this->user, 5);
    assertEqual($this->user->funds, 5.5);
});

it('can add funds');

r user has funds → This test did not perform any assertions /Users/alcivaw/Projects/myapp/vendor/pestphp/pest/src/Factories/TestCaseFactory.php(185) : eval()'d code:4 ✓ can subtract funds p can add funds

[fetzi]

This is the standard behavior of xUnit testing frameworks. I'm also not sure what this test case should do? 🤔

[miscbits]

This was something we were talking about on the discord to replicate the behavior in mocha.js
https://mochajs.org/#pending-tests

Which test are you asking about? There are three in the example provided

[Gummibeer]

Isn't this the same/similar to #92?

[miscbits]

@Gummibeer yes. This has just been sitting for a while is all cause I didn't really have the time to work on it myself.

[AlexMartinFR]

It seems I posted a duplicate issue indeed.

I have a PR on the way to take care of that.

Empty tests will now be marked as incomplete:

[AlexMartinFR]

Update: my PR isn't ready, I found some implementation problems. If you want to help, here is what I have so far.

---

Problem:

By default, PHPUnit marks both empty & assertion-less tests as Risky.
We want a slightly different behavior to improve the Pest UX.

What we want:

• An empty test must be marked as Incomplete
• A non-empty test without any assertion must still be marked as Risky

Technically, we want to inject PHPUnit's markTestIncomplete('wip') inside empty tests.

Our options:

A. Chainable method: ->wip()

The user can manually mark his test as Incomplete by chaining the wip() method:

test('future feature')->wip();

This is the easiest way to achieve a visual result similar to my previous screenshot.
Here is the implementation:

// pest/src/PendingObjects/TestCall.php

/**
 * Marks the current test as incomplete.
 *
 * @param string $message
 */
public function wip(string $message = 'wip'): TestCall
{
    $this->testCaseFactory
        ->chains
        ->add(Backtrace::file(), Backtrace::line(), 'markTestIncomplete', [$message]);

    return $this;
}

B. Automatic injection in empty tests:

The ideal solution. But I'll need help for this one.

We want to detect when a test is empty, and inject markTestIncomplete('wip').

1. Detecting empty higher order tests:

test('future feature');

A higher order test has no provided closure, but returns an instance of _NullClosure_ instead.
Its code and assertions will be chained, so an empty test will have an empty chain.

NullClosure + empty chained message collection = empty Higher Order Test.

1. Detecting empty closure tests:

test('future feature', function () {
});

So far, I haven't found any way to read the content of the closure provided by the user.
If we can access it somehow, we could know if it's an empty test or not.

TL:DR;

I could submit a PR right now to add the ->wip() method, if everyone feels it's a good solution.
Otherwise you help is welcome to make progress on B. Automatic injection.

[owenvoke]

Personally I feel like ->incomplete() would be better as the main call. And perhaps having a ->wip() call that passes the values to ->incomplete() with a default message of "wip". So wip() is just a shorthand and then incomplete() ties in with the underlying PHPUnit naming.

Example methods

For example:

/**
 * Marks the current test as incomplete.
 *
 * @param string $message
 */
public function incomplete(string $message): TestCall
{
    $this->testCaseFactory
        ->chains
        ->add(Backtrace::file(), Backtrace::line(), 'markTestIncomplete', [$message]);

    return $this;
}

/**
 * Marks the current test as incomplete.
 *
 * @param string $message
 */
public function wip(string $message = 'wip'): TestCall
{
    return $this->incomplete($message);
}

However, I love the wip() call and would probably use that all the time anyway, so it doesn't really matter. :+1:

[AlexMartinFR]

So on Discord, Nuno mentioned another interesting solution:

We could have a new wip('') test method:

wip('future feature', function () {
    // …
});

Pest would not run these tests at all, unless we call the --wip option:

./vendor/bin/pest --wip

We would gain a cleaner reporting by default. Thoughts?

[AlexMartinFR]

We have different choices here, let's sum them up:

Syntax:

A. Shorter syntax:

wip('future feature');

B. Chainable syntax:

test('future feature')->wip();

Behavior:

1. Skip wip tests by default.
   ./vendor/bin/pest --wip will show wip tests only.

2. Skip wip tests by default.
   ./vendor/bin/pest --wip will show wip tests and normal tests.

[nunomaduro]

No plans to have this.