mislant commented 2 years ago

More information: If you uses specify out of box (as described in github page), it will throw an TypeError: Codeception\Specify\SpecifyTest::run(): Return value must be of type PHPUnit\Framework\TestResult, null returned

I have looked in and noticed that TestResult object was got from TestCase object. That how it works in 4th version. And codeception runs test using TestCase::run method. But in 5th version it uses TestCaseWrapper. That's why there is not TestResult object in TestCase to give it in Specify

Codeception has default configurations (just after codecept bootstrap)

Naktibalda commented 2 years ago

It looks like 3 separate implementations will be required to support Codeception 5, PHPUnit 10 and older versions.

Signature of TestCase::run method is different in PHPUnit 10 and TestResult class is marked for removal.

Naktibalda commented 2 years ago

Removing SpecifyTest::run method may make it work with Codeception 5, could you test it?

mislant commented 2 years ago

I don't think that removing SpecifyTest::runwould solve the problem, because there is no other way to run test by Specify. Standard trait Specify gives BDD interface (specify, should etc.) and all implementation calls runSpec of SpecifyHooks and this method wrap callable assertions in SpecifyTest object, which implements PHPUnit\Framework\Test interface

    private function runSpec(string $specification, Closure $callable = null, $params = [])
    {
        // other code
        $test = new SpecifyTest($callable->bindTo($this));
        // other code

        // run test
        $test->run($this->getTestResultObject());
       // done
    }

Looks like there is no way to use Specify in Codeception 5. I tried to change implementation of SpecifyHooks to force them to use ResultAggregator instead of TestResult, But it's not impossible. ResultAggregator methods works with FailureEvent which needs abstract class Test, which is TestCaseWrapper.

mislant commented 2 years ago

Removing SpecifyTest::run method may make it work with Codeception 5, could you test it?

Naktibalda commented 2 years ago

Sorry, it was a very bad guess, I haven't even looked at the code and guessed that it calls parent::run().

Do you understand what's the point of catching AssertionFailedError in SpecifyTest::run method. Is it to show the name of specific specification in report?

According to #53 failed spec appears as failed test, but parent test appears successful.

✖ FailedTest: With specify | specification | examples index 0 
✔ FailedTest: With specify (0.01s)

Removing catch would change output to

✖ FailedTest: With specify (0.01s)

And it would actually fix #53

Do you see any downsides?

It may be necessary to move cleanup code in SpecifyHook::runSpec method to finally block https://github.com/Codeception/Specify/blob/7dc67c1d9d9b6d6dd29af65011540311c0a0bbc6/src/Codeception/Specify/SpecifyHooks.php#L79-L86

mislant commented 2 years ago

Sorry, it was a very bad guess, I haven't even looked at the code and guessed that it calls parent::run().

Do you understand what's the point of catching AssertionFailedError in SpecifyTest::run method. Is it to show the name of specific specification in report?

According to #53 failed spec appears as failed test, but parent test appears successful.
✖ FailedTest: With specify | specification | examples index 0 
✔ FailedTest: With specify (0.01s)
Removing catch would change output to
✖ FailedTest: With specify (0.01s)
And it would actually fix #53

Do you see any downsides?

It may be necessary to move cleanup code in SpecifyHook::runSpec method to finally block

https://github.com/Codeception/Specify/blob/7dc67c1d9d9b6d6dd29af65011540311c0a0bbc6/src/Codeception/Specify/SpecifyHooks.php#L79-L86

It's good temporary solution. It makes all my tests run on 5th Codeception, but at same time it has some cons. Solution:


// SpecifyTest::run()
public function run(TestResult $result = null): TestResult
{
    call_user_func_array($this->test, $this->example);
    return new TestResult();
}

// SpecifyHooks::runSpec()
try {
        $test->run($this->getTestResultObject());
} finally {
    $this->specifyCheckMockObjects();

    // restore object properties
    $this->specifyRestoreProperties($properties);

    if (!empty($this->afterSpecify) && is_array($this->afterSpecify)) {
        foreach ($this->afterSpecify as $closure) {
            if ($closure instanceof Closure) {
                $closure->__invoke();
            }
        }
    }
}

Pros:

Tests run

Cons:

Output of tests is different. There is more information and interesting style of output in default implementation. It's not critical but still.


// FIXED 5TH VERSION OUTPUT

Tests.Unit Tests (2) ---------------------------------------
✔️ ExampleTest: Some feature (0.01s)
✖️ SomeTest: Sum (0.00s)
------------------------------------------------------------
Time: 00:00.036, Memory: 4.00 MB

There was 1 failure: 1) SomeTest: Sum Test tests/Unit/SomeTest.php:testSum Failed asserting that 1 matches expected 2.

1 /home/kkaliev@wooppay.kz/tmp/codeception/tests/Unit/SomeTest.php:12

2 SomeTest->{closure}

3 /home/kkaliev@wooppay.kz/tmp/codeception/tests/Unit/SomeTest.php:13

FAILURES! Tests: 2, Assertions: 2, Failures: 1.

Process finished with exit code 1

// 4TH VERSION DEFAULT OUTPUT

Unit Tests (2) --------------------------------------------- ✔️ ExampleTest: Some feature (0.01s) ✖️ testSum | Some class sum (0.01s) ✔️ SomeTest: Sum (0.00s)

Time: 00:00.028, Memory: 4.00 MB

There was 1 failure:

1) testSum | Some class sum Test vendor/codeception/specify/src/Codeception/Specify/SpecifyTest.php:testSum | Some class sum Failed asserting that 2 matches expected 3.

1 /home/kkaliev@wooppay.kz/tmp/codeception-v4/tests/Unit/SomeTest.php:12

2 SomeTest->{closure}

3 /home/kkaliev@wooppay.kz/tmp/codeception-v4/tests/Unit/SomeTest.php:13

FAILURES! Tests: 2, Assertions: 2, Failures: 1.

Process finished with exit code 1


 As you mentioned it also solves problem of #53 issue.

  - If you have nested callables with more then one test block, failure in the first one would broke all case, and nexts wouldn't be run. On my point this is the most biggest problem.
  - Solution makes code of creating specification string useless, because you can't see it in failure output (maybe it's because my understanding of framework not good enough to make it)

I think we need to create another different solution for 5th version, to implement specification output and correct work with new environment.

Naktibalda commented 2 years ago

The output difference could be solved by prepending specify name to exception message.

Continuing execution of other examples is more complicated.

To support Specify, Codeception 5 must expose ResultAggregator. Probably the best option would be to make Codecept::getResultAggregator method static.

try {
    call_user_func_array($this->test, $this->example);
} catch (AssertionFailedError $error) {
    if (class_exists(\Codeception\Codecept::class) && \Codeception\Codecept::getResultAggregator() !== null
) {
        \Codeception\Codecept::getResultAggregator()->addFailure(
            new \Codeception\Event\FailEvent(
                new \Codeception\Test\TestCaseWrapper(
                    clone($this)
                ),
                $error,
                0
            );
        );
    } else {    
        $result->addFailure(clone($this), $error, $result->time());
    }
}

mislant commented 2 years ago

I see Codeception's Unit class has setResultAggregator but there is no one place where this method would be called. I'm not sure, but what if we add in Codeception\Test\Test abstract method setResultAggregator. And use it in realRun:

Make this:

    final public function realRun(ResultAggregator $result): void
    {
        $this->resultAggregator = $result;

Like this

    final public function realRun(ResultAggregator $result): void
    {
        $this->setResultAggregator($result);

And in implementation in TestCaseWrapper we could write something like this:

    public function setResultAggregator(?ResultAggregator $resultAggregator): void
    {
        $this->resultAggregator = $resultAggregator;
    $this->testCase->setResultAggregator($resultAggregator)
    }

And we will be able to use ResultAggregator in our tests

mislant commented 2 years ago

I understand that it may be not perfect solution but i just want to show direction of my mind. I don't think making ResultAggregator global accessible is good idea

Naktibalda commented 2 years ago

Unit::setResultAggregator is dead code since introduction of TestCaseWrapper. It would still be a Specify specific solution and look like dead code, so it can be removed by accident.

This solution wouldn't support tests extending PHPUnit\Framework\TestCase directly.

mislant commented 2 years ago

Bad situation( At least your solution helped me to run Specify using Codeception 5. I'll hope framework will choose stable concepts that can be used natively for this lib

Naktibalda commented 2 years ago

There will be no TestResult in PHPUnit 10: https://github.com/sebastianbergmann/phpunit/blob/c7c1903ce2995f36a016a245d43ca21ea8ea352f/src/Framework/Test.php#L19

Codeception / Specify

Type error when using Codeception 5th version #60

1 /home/kkaliev@wooppay.kz/tmp/codeception/tests/Unit/SomeTest.php:12

2 SomeTest->{closure}

3 /home/kkaliev@wooppay.kz/tmp/codeception/tests/Unit/SomeTest.php:13

Unit Tests (2) --------------------------------------------- ✔️ ExampleTest: Some feature (0.01s) ✖️ testSum | Some class sum (0.01s) ✔️ SomeTest: Sum (0.00s)

1 /home/kkaliev@wooppay.kz/tmp/codeception-v4/tests/Unit/SomeTest.php:12

2 SomeTest->{closure}

3 /home/kkaliev@wooppay.kz/tmp/codeception-v4/tests/Unit/SomeTest.php:13