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

@Depends file will be run if not ran yet + Full namespace bug when using @depends [FEATURE&BUG-FIX] #3449

Closed

Conversation

NikoGrano
Copy link

@NikoGrano NikoGrano commented Dec 11, 2018

#2647

As @sebastianbergmann said:

That being said, yes, @Depends can be used to denote a dependency on a test that is declared in another test case class.
Keep in mind, though, that PHPUnit cannot ensure that a depended-upon test is executed before the test(s) that depend(s) on it.

This PR will now ensure depended-upon test is executed before the test(s) that depend(s) on it.

Some stuff to note:

  • If you're depended-upon test which is failed, your test will be marked as skipped with warning: This test depends on "%s" to pass.
  • If you're depended-upon test which is skipped, your test will be marked as skipped with warning: This test depends on "%s" to pass.

Fix:
In case of you're writing @depends \Some\Project\Tests\Unit\Domain\ProductTest::test__toString this will fail with message This test depends on "%s" to pass.. Why? Due the \ at beginning of the string... I fixed this with following code:

                if (\strpos($dependency, '\\') === 0)
                {
                    $dependency = \substr($dependency, 1);
                }

PS. I had issues creating tests for these changes. Can somebody show example or create tests?
PPS. Would it be possible to get this feature into 7.5 or 7.6 etc..?

@codecov
Copy link

codecov bot commented Dec 11, 2018

Codecov Report

Merging #3449 into master will increase coverage by 0.11%.
The diff coverage is 95.28%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3449      +/-   ##
============================================
+ Coverage     82.15%   82.26%   +0.11%     
- Complexity     3580     3601      +21     
============================================
  Files           143      143              
  Lines          9398     9457      +59     
============================================
+ Hits           7721     7780      +59     
  Misses         1677     1677
Impacted Files Coverage Δ Complexity Δ
src/Framework/TestCase.php 77.02% <95.28%> (+1.43%) 325 <38> (+21) ⬆️
src/TextUI/Command.php 71.74% <0%> (+0.34%) 208% <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 b8f38d3...c33a58a. Read the comment docs.

@epdenouden
Copy link
Contributor

@Niko9911 Oh nice, more test execution order features :-) I will have a look for sure

NikoGrano added a commit to NikoGrano/phpunit that referenced this pull request Dec 11, 2018
@NikoGrano
Copy link
Author

There is also issue when returning value from depended-upon test. It will always return null when using data sets. I'm currently working on this issue and will commit the fix into this PR.

Meanwhile, please approve ;)

@NikoGrano
Copy link
Author

I think it would be best if this could be merged into 7.5?

$failedKeys = [];

foreach ($failed as $failure) {
$pos = \strpos($failure->getTestName(), ' with data set');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The substring with [data set] is dependent on how test names are send to the loggers. This could change in the future, for example with #3439

You can retrieve the TestCase via $failure->failedTest() and explore if it has a data set using $test->dataName()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is the way how it is used elsewhere also.

At that point only thing what I'm interested about is \some\namespace\class::someFunction

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used like this elsewhere indeed! Which keeps giving me headaches as I am trying to refactor it out. 😆 For the test execution reordering and TestDox original-ordering, I cooked up this one:

public static function getTestSorterUID(Test $test): string
{
if ($test instanceof PhptTestCase) {
return $test->getName();
}
if ($test instanceof TestCase) {
$testName = $test->getName(true);
if (\strpos($testName, '::') === false) {
$testName = \get_class($test) . '::' . $testName;
}
return $testName;
}
return $test->getName();
}

The with data set substring is a user interface element that should be independent of the inner workings of the test runner. Here's an upcoming change that might interfere with your code: 06d8c4b

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey, I'm still not sure how you're gonna replace that "with data set". You would need also refactor these

private function handleDependencies(): bool
{
if (!empty($this->dependencies) && !$this->inIsolation) {
$className = \get_class($this);
$passed = $this->result->passed();
$passedKeys = \array_keys($passed);
$numKeys = \count($passedKeys);
for ($i = 0; $i < $numKeys; $i++) {
$pos = \strpos($passedKeys[$i], ' with data set');
if ($pos !== false) {
$passedKeys[$i] = \substr($passedKeys[$i], 0, $pos);
}
}
$passedKeys = \array_flip(\array_unique($passedKeys));
foreach ($this->dependencies as $dependency) {
$deepClone = false;
$shallowClone = false;
if (\strpos($dependency, 'clone ') === 0) {
$deepClone = true;
$dependency = \substr($dependency, \strlen('clone '));
} elseif (\strpos($dependency, '!clone ') === 0) {
$deepClone = false;
$dependency = \substr($dependency, \strlen('!clone '));
}
if (\strpos($dependency, 'shallowClone ') === 0) {
$shallowClone = true;
$dependency = \substr($dependency, \strlen('shallowClone '));
} elseif (\strpos($dependency, '!shallowClone ') === 0) {
$shallowClone = false;
$dependency = \substr($dependency, \strlen('!shallowClone '));
}
if (\strpos($dependency, '::') === false) {
$dependency = $className . '::' . $dependency;
}
if (!isset($passedKeys[$dependency])) {
$this->status = BaseTestRunner::STATUS_SKIPPED;
$this->result->startTest($this);
$this->result->addError(
$this,
new SkippedTestError(
\sprintf(
'This test depends on "%s" to pass.',
$dependency
)
),
0
);
$this->result->endTest($this, 0);
return false;
}
if (isset($passed[$dependency])) {
if ($passed[$dependency]['size'] != \PHPUnit\Util\Test::UNKNOWN &&
$this->getSize() != \PHPUnit\Util\Test::UNKNOWN &&
$passed[$dependency]['size'] > $this->getSize()) {
$this->result->addError(
$this,
new SkippedTestError(
'This test depends on a test that is larger than itself.'
),
0
);
return false;
}
if ($deepClone) {
$deepCopy = new DeepCopy;
$deepCopy->skipUncloneable(false);
$this->dependencyInput[$dependency] = $deepCopy->copy($passed[$dependency]['result']);
} elseif ($shallowClone) {
$this->dependencyInput[$dependency] = clone $passed[$dependency]['result'];
} else {
$this->dependencyInput[$dependency] = $passed[$dependency]['result'];
}
} else {
$this->dependencyInput[$dependency] = null;
}
}
}
return true;
}

public function getDataSetAsString(bool $includeData = true): string
{
$buffer = '';
if (!empty($this->data)) {
if (\is_int($this->dataName)) {
$buffer .= \sprintf(' with data set #%d', $this->dataName);
} else {
$buffer .= \sprintf(' with data set "%s"', $this->dataName);
}
$exporter = new Exporter;
if ($includeData) {
$buffer .= \sprintf(' (%s)', $exporter->shortenedRecursiveExport($this->data));
}
}
return $buffer;
}

I'm still not sure how these should be refactored.

Also I'm not sure how I can access dependencies of test inside of TestSuiteSorter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's enough to refactor for everyone! Don't be shy, just grab a few classes and have at it. 🛠

You can get the name of a TestCase with:

  • $testCase->getName($withDataSet) which gives you the name with(out) a prettified data set
  • $testCase->dataName() returns the name of the data set you see in getName()
  • $testCase->getDependencies() returns a list of dependencies, if any

Please explore the surrounding code a bit more and write tests before you make such large changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand and I would like to fix things. However I want remind about few facts.

I'm using heavily my own time to fix & learn this stuff. It's not as simple as it looks for guy who have no experience with PHPUnit core. I will look into this after I have created some tests.

Would need this feature ASAP into PHPUnit...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it, no worries! Like many other tools and frameworks PHPUnit is maintained by volunteer contributors. We are paid in emoji and sleepless nights.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epdenouden Tests are now Implemented 🎉

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epdenouden Btw, cannot really remove this $pos = \strpos($failure->getTestName(), ' with data set');

Because if taking look into src/Framework/TestFailure.php there will be

/**
     * Constructs a TestFailure with the given test and exception.
     *
     * @param Throwable $t
     */
    public function __construct(Test $failedTest, $t)
    {
        if ($failedTest instanceof SelfDescribing) {
            $this->testName = $failedTest->toString();
        } else {
            $this->testName = \get_class($failedTest);
        }

        if (!$failedTest instanceof TestCase || !$failedTest->isInIsolation()) {
            $this->failedTest = $failedTest;
        }

        $this->thrownException = $t;
    }

Same with other stuff. Meaning this needs to be refactored and refactoring this means refactoring also skipped, passed, etc... Would change little bit too much logic to be in this PR.

) {
$dependencyClass = \explode('::', $dependency, 2)[0];
(new TestSuite($dependencyClass, 'Dependency_' . \ucfirst($dependencyClass)))
->run($this->result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😨 This is scary bit of code. It fires off a lot of logic that will have unforeseen consequences, I'm just not sure which at the moment.

If I am reading this correctly, this code does the following:

  1. if the $dependency has not been passed yet, do
  2. check if $dependency hasn't been failed or skipped
  3. get the name of the class in which the dependency resides by splitting the FQCN
  4. yolo (new TestSuite($theDependencyWeNeed))->run($currentResult)

I get what you are trying to achieve: auto-run dependencies without any hassle on the user side. If this logic were to be accepted, it needs more safeguards and refactoring:

  • what are the side effects of instantiating a TestSuite here in this scope?
  • what happens if this TestSuite has already been instantiated, but: not run? or not run completely? failed during either setUpBeforeClass or setUp?
  • even if this works, handleDependencies() shouldn't just ->run() the missing dependency and merge it into the current result. Leave that responsibility to the Runner and find a cleaner way to pass around the execution plan.

Tests, tests and more tests. Preferably a few end-to-end ones :-)

Copy link
Author

@NikoGrano NikoGrano Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is scary bit of code. It fires off a lot of logic

I know... Kinda the way to deal with it, when I cannot access the list of tests to execute. Another solution would be scanning all files which depends on which one and reorder them. However, I tried it without success.

If I am reading this correctly, this code does the following:

Yup.

what are the side effects of instantiating a TestSuite here in this scope?

Yup...
image

what happens if this TestSuite has already been instantiated, but: not run? or not run completely? failed during either setUpBeforeClass or setUp

Good question. I will check what happens.

Tests, tests and more tests. Preferably a few end-to-end ones :-)

Not sure how to implement those at all...

even if this works, handleDependencies() shouldn't just ->run() the missing dependency and merge it into the current result. Leave that responsibility to the Runner and find a cleaner way to pass around the execution plan.

More specific implementation how to do it?
I could just run it like this and remove the result from wrong place like shown in the image?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this issue showing in the image has been fixed..

@epdenouden
Copy link
Contributor

@Niko9911 I will reply more tomorrow, logging off for the day.

@NikoGrano
Copy link
Author

@epdenouden NP. Thanks for helping. I will try resolve some dangerous bugs meanwhile when using stuff chained....

@NikoGrano
Copy link
Author

Okey, I have done everything I could. I tested them with my project. Tests could be nice feature to add here, but I have no Idea how to create them....

Plz Review 😃

@NikoGrano
Copy link
Author

NikoGrano commented Dec 11, 2018

Seems like 1021 test makes sure this will not fail... Ah crap, I'll fix it...

@NikoGrano
Copy link
Author

Tests are now passing. 😏

@epdenouden
Copy link
Contributor

@Niko9911 Quick glance: what test has been added that tests the functionality you added? Did you add a test that failed first and now works? Changes in the core runner requires strong testing, especially to guard against edge cases.

@NikoGrano
Copy link
Author

@epdenouden There is no tests created for this functionality. I will try create some today.
As I still see, most of the code is already tested with current tests.

I will be posting tests today/tomorrow into this PR. Could you review the code meanwhile. (The code is already tested in our project using many edge cases so it should work. But as we all know, that is not enough and I will try reproduce those cases as tests and implement those tests into this PR.)

@epdenouden
Copy link
Contributor

@Niko9911 Keep in mind that PHPUnit is a tool used by many different projects and environments. If it works in your project that's... a good start. :-)

By the way, which project is it and it is publicly available somewhere? I am still very curious what the actual use case is. Depending on "[...] a test that is declared in another test case class" (as @sebastianbergmann says in#2647) makes me curious what other mechanism for sharing dependencies could solve your problem.

@NikoGrano
Copy link
Author

@epdenouden Sorry, I had to blur even the project names in previous images. It's private and under strict NDA. Would like share code, but as I think you understand, it's out of my hands if I can share it or not.

The idea is to check if x tests was success fully working, so test y can be tested. For example, if Domain tests fail, commandHandler or Infra tests depending on domain test will be marked skipped, until domain tests will pass.

@NikoGrano
Copy link
Author

@sebastianbergmann Could it be possible to implement this into 7.5.x?

@NikoGrano
Copy link
Author

I kinda found another bug 💤 if depending upon other class and it is not executed yet, it works, but if it's in same class as your test function, it will fail as the function is not executed yet. Reordering functions in class works.

I'm gonna fix this also, but it's more dangerous... (High possibility for infinite loop)

@epdenouden
Copy link
Contributor

It's private and under strict NDA. Would like share code, but as I think you understand, it's out of my hands if I can share it or not.

Then make sure you have a stand-alone and self-contained test for this scenario in your proposed code.

The idea is to check if x tests was success fully working, so test y can be tested. For example, if Domain tests fail, commandHandler or Infra tests depending on domain test will be marked skipped, until domain tests will pass.

That makes sense. You are looking for what is basically the SKIPIF section of an end-to-end test. This is not an uncommon request and there are multiple ways of doing that to effectively create a custom @requires annotation, by way of logic in setUp(). A recent issue here that touches on this was #3427. Also, search for 'skip' in the issues list here, there are many "how to skip test using annotations" questions.

The current solution you are proposing doesn't just "skip if other tests haven't run successfully yet", it actively changes the state of the test runner while this is running. 😨 If you want a solution based on test reordering you will always end up with hacky solutions like this, unless you want to refactor more internals.

@NikoGrano
Copy link
Author

@epdenouden

I kinda found another bug 💤 if depending upon other class and it is not executed yet, it works, but if it's in same class as your test function, it will fail as the function is not executed yet. Reordering functions in class works.

This won't fix due the basic logic behind the PHPUnit. I will mark tests as skipped with following msg: Reordering same class dependency function is not implemented. Please reorder "%s" before "%s".

If you want a solution based on test reordering you will always end up with hacky solutions like this, unless you want to refactor more internals.

True, Thats why I have to propose hacky solution, but it is working. Also refactoring internals would mean basically refactoring ½ test framework, which would mean it's better to start from scratch and decide what copy from "old" framework to new.

multiple ways of doing that to effectively create a custom @requires annotation, by way of logic in setUp().

But would make much more sense to implement this in level on TestFramework.

actively changes the state of the test runner while this is running.

What you mean? Could you elaborate?

If this is not possible to implement (merge) into the PHPUnit, could it be possible to make TestCase expendable in this manner?

@NikoGrano
Copy link
Author

NikoGrano commented Dec 12, 2018

Travis.... ComeON! (Random error)

@epdenouden
Copy link
Contributor

epdenouden commented Dec 12, 2018

Will reply more later, but for now: I don't agree with just shoving in a hacky solution without proper tests. It can be done properly. You'd have to read up on the functionality and limitations of test reordering.

I disagree fundamentally that it makes sense to implement a quickfix in a generalized testing framework, instead of in an extended TestCase, setUp() or even a Packagist package that patches PHPUnit's files.

The test runner core is like a watch or a clock. I do not understand, at all, how you can just propose creating and running a TestSuite there in the middle of a dependency check.

@NikoGrano
Copy link
Author

but for now: I don't agree with just shoving in a hacky solution without proper tests

I'm working on tests... I just cannot use those I used in our project. Hold on... Tests are coming...

how you can just propose creating and running a TestSuite there in the middle of a dependency check.

To not refactor everything. If you agree with this feature, you could implement it better as you can see, my knowledge isn't enough to any deeper...

I disagree fundamentally that it makes sense to implement a quickfix in a generalized testing framework, instead of in an extended TestCase, setUp() or even a Packagist package that patches PHPUnit's files.

How does one implement this into setUp()?
Patches also mean if anything changes in PHPUnit, the patch will fail.
How I see it, even if this is generalized testing framework and as long it doesn't affect (cause bugs) and is properly tested and providing new features, why not to implement it into the framework?

@sebastianbergmann I'm highly waiting for you opinion about this, if this is worth to spent time?

@NikoGrano
Copy link
Author

From my perspective this PR is ready to be merged.

@epdenouden
@sebastianbergmann

$dependencyClass = \explode('::', $dependency, 2)[0];
$dependencyKey = self::DEPENDENCY_PREFIX . \ucfirst($dependencyClass);
$oldErrorsCount = self::$DEPENDENCY_TASK_RESULTS->errorCount();
(new TestSuite($dependencyClass, $dependencyKey))->run(self::$DEPENDENCY_TASK_RESULTS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before you put any more work into this, wait for a reply for @sebastianbergmann

The design of this feature still worries me. Creating a new TestSuite for the missing dependenc(y|ies) and run() it while running the main loop, is the least optimal way of solving this. If you want to run @depends before the dependant test, make sure the tests are in the optimal order before you start running.

  • what protection is there against instantiating (and running) the same TestSuite twice?
  • ...or more than twice? are results from the @depends test merged back into the 'main' $testResult
  • how does all this work out when sent to the various loggers? Some of them, like JUNit, log the TestSuite hierarchy as the start*/add*/end* events are fired

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what protection is there against instantiating (and running) the same TestSuite twice

It will be ran twice, if it is not in correct order.

...or more than twice? are results from the @Depends test merged back into the 'main' $testResult

Not possible to run it over twice. The result will be put into dependency-only testResult in case it's required more than one time before it is ran in correct order.

how does all this work out when sent to the various loggers? Some of them, like JUNit, log the TestSuite hierarchy as the start*/add*/end* events are fired

I'll test this. Should not affect that.

private const DEPENDENCY_PREFIX = 'Dependency_';

/** @var TestResult \PHPUnit\Framework\TestResult */
private static $DEPENDENCY_TASK_RESULTS;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This data structure is only needed for the edge case of automatically running dependencies located in another class. Does this need to be a class-wide variable?
Also: why static, it belongs to an actual TestCase instance?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's static for optimization reasons.

Meaning, if multiple functions require this same dependency, the results will be used from this class property instead of running dependency again and again, maybe even 100 times, before it will run the test.

$_SERVER['argv'][4] = '--order-by=no-depends,reverse';
$_SERVER['argv'][5] = 'MultiDependencyTest';
$_SERVER['argv'][6] = __DIR__ . '/../_files/MultiDependencyTest.php';
$_SERVER['argv'][4] = '--reverse-order';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use --order-by= CLI flag instead of legacy --reverse-order and --ignore-dependencies

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Will fix this.

@epdenouden
Copy link
Contributor

@Niko9911 I can have a more detailed look this weekend. The current design of creating+running a new TestSuite for the missing dependency is a hack and should be handled before the run starts. It would take time to brainstorm and review possible consequences of the current approach. The collection of tests that comes with PHPUnit didn't have to guard against this in the past and it isn't surprising the main test suites went green so easily.

My vote would be for this change being an external script, like Symfony's PHPUnit bridge. Or implement it like the TestSuiteSorter: external to the main Runner.

(Context: after two rounds of clean up of the TestDox logger, I am still finding edge cases and yesterday a bug that throws a PHP error.)

@NikoGrano
Copy link
Author

PHPUnit didn't have to guard against this in the past and it isn't surprising the main test suites went green so easily.

There is new tests for this logic. As I use this now heavily with edge cases 7 days/week currently, I can quickly identify and fix these issues. I've been implementing as much tests as I can.

@sebastianbergmann Would really require your opinion about this! Is this possible to be merged into PHPUnit & What is your opinion?

@NikoGrano
Copy link
Author

@sebastianbergmann @epdenouden Any news?

@epdenouden
Copy link
Contributor

Not from me, no

@epdenouden
Copy link
Contributor

@Niko9911 Not sure how busy @sebastianbergmann is at the moment, it's Xmas season and all that.

As for the cross-class @depends support: I have taken a look at the current mechanism of TestSuiteSorter::resolveDependencies and what changes would be required. It is possible, e.g. by hacking in a quick lookup list. The PHPUnit runner has no clue about the tree structure of the suites and tests. Without it anything will be a patched solution at best.

I will be adding full support for 'interdependent units of work' in the near future. However, a clean and maintainable solution is required there, as that refactoring relates to solving @runClassInSeparateProcess and related isolation infrastructure.

@NikoGrano
Copy link
Author

@epdenouden So you basically say I should drop this current proposed implementation and go with TestSuiteSorter::resolveDependencies?

@sebastianbergmann I'm not gonna stop mentioning you before you reply. Just to keep reminding you about this PR. 😜

@epdenouden
Copy link
Contributor

epdenouden commented Dec 21, 2018

@Niko9911 TL;DR: I agree inter-class @depends is a good idea, but I fear your proposed implementation will cause more edge cases in the future.

Mind you, this isn't my personal project. I just enjoy working on it and being useful around the yard. Not sure what to tell you, as you have mentioned before you do not mind a hacky temporary solution. If that is what you need right now and it solves your problems, my advice would be to make your change a private patch for PHPUnit and publish it on Packagist.

I mentioned the TestSuiteSorter because it turned out to be rather complex to fiddle with the test running order. There are just so many scenarios to keep in mind. Currently in the middle of refactoring the collection of end-to-end tests and all these are just to cover known use cases: https://github.com/epdenouden/phpunit/tree/improved-testdox/tests/end-to-end/execution-order

Your proposed change will run missing dependencies on the spot, while the existing executio reordering does so before the run. Also it doesn't force-run missing dependencies. I am not sure yours does even.

Anyway. I have a few other loose ends to tie up, I'll wait and see what @sebastianbergmann thinks.

@sebastianbergmann
Copy link
Owner

Replaced by #3519.

@epdenouden
Copy link
Contributor

This functionality has been inplemented as a part of #3936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants