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

Using @depends to depend on classes #3519

Closed
sebastianbergmann opened this issue Feb 7, 2019 · 9 comments · Fixed by #3936
Closed

Using @depends to depend on classes #3519

sebastianbergmann opened this issue Feb 7, 2019 · 9 comments · Fixed by #3936
Assignees
Labels
feature/test-dependencies Issues related to explicitly declared dependencies between tests type/enhancement A new idea that should be implemented

Comments

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Feb 7, 2019

As of today, the @depends annotation cannot be used to express that a test (or all tests of a test case class) depend on all the tests of another test case class to pass. This fact is not clear in the documentation (sebastianbergmann/phpunit-documentation-english#121 exists to remedy this). It is also not clear from the error output you get if you try to use @depend TestClassName. Consider the two test case classes shown below:

<?php declare(strict_types=1);
use PHPUnit\Framework\TestCase;

/**
 * @depends XyzTest
 */
final class AbcTest extends TestCase
{
    public function testOne(): void
    {
        $this->assertTrue(true);
    }
}
<?php declare(strict_types=1);
use PHPUnit\Framework\TestCase;

final class XyzTest extends TestCase
{
    public function testOne(): void
    {
        $this->assertTrue(true);
    }
}

Executing the tests shown above results in the confusing output shown below:

$ ./phpunit
PHPUnit 8.0.1 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.3.2 with Xdebug 2.7.0rc1
Configuration: /home/sb/example/phpunit.xml

S.                                                                  2 / 2 (100%)

Time: 35 ms, Memory: 4.00MB

There was 1 skipped test:

1) AbcTest::testOne
This test depends on "AbcTest::XyzTest" to pass.

OK, but incomplete, skipped, or risky tests!
Tests: 2, Assertions: 1, Skipped: 1.

Once #3517 is implemented this error output would be less confusing. But it would still not be clear that using @depends to depend on all tests of a test case class to pass is not possible.

In the past I said that "[i]t does not make sense to me for a test to depend on all tests of a test case class". In a recent discussion, @spriebsch changed my mind. Especially in the context of integration tests, or larger tests in general, this makes a lot of sense.

What does not make sense, in my opinion, is to pass the results of all tests of a test case class to the test(s) that depend(s) on that test case class.

@sebastianbergmann sebastianbergmann added the type/enhancement A new idea that should be implemented label Feb 7, 2019
@sebastianbergmann
Copy link
Owner Author

@epdenouden Would be great to get your input on this.

@spriebsch
Copy link

Just to give an example for where I think depending on a test class makes sense: in a project, I have integration tests covering each step of a process that the software implements. Each step is based on the result of the previous step, so expressing that by a test dependency seems appropriate. Rather than depending on a certain test method, however, I would want to depend on the whole test class responsible for a given process step.

@epdenouden
Copy link
Contributor

epdenouden commented Feb 9, 2019

Completely missed this new issue being referenced in #3517 and fits right in with all the other thinking about what 'independent units of work' a test collection is made out of. The scenario @spriebsch mentions is a daily business in my work.

I will have a good look at TestCase::handleDependencies() and do some exploration. I think this should be doable and safe, as long as we are not getting into the risky territory of reordering-while-running like #3449.

@epdenouden
Copy link
Contributor

@sebastianbergmann you can just assign the issue to me if you want :)

@sebastianbergmann
Copy link
Owner Author

"I don't think this should be doable and safe" scares me.

@epdenouden
Copy link
Contributor

epdenouden commented Feb 9, 2019

And rightfully so! I was doing github-before-coffee. Apologies @sebastianbergmann, I mixed two topics in one sentence:

  • completely safe and doable: doing a check on a full class @depends and throwing a warning
  • risky with sideeffects: doing a check on @depends and reordering while running

@epdenouden
Copy link
Contributor

As you might have seen there was some discussion around forcing dependencies to run. Basically that could work, seeing as the result returned simply comes from the ['result']. Trouble starts when you have to check things like...

  • has the dependency been filtered? (and is the skip/warn intentional)
  • did the dependency simply fail?
  • do external processes make this harder? I still have to look at @runClassInSeperateProcess
  • etc

So yeah. Let's avoid such risks around the runner core :)

@barbu110
Copy link

What's the status of this, please?

@NikoGrano
Copy link

Bump #3449

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/test-dependencies Issues related to explicitly declared dependencies between tests type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants