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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 16 commits into from
Closed

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

Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6c50e02
Depends file will be run if not ran yet.
NikoGrano Dec 11, 2018
3772bad
Fixed issue: https://github.com/sebastianbergmann/phpunit/issues/3450
NikoGrano Dec 11, 2018
06a277e
CS Fixer
NikoGrano Dec 11, 2018
64d6350
Fixes: If SetUp Fails, If There is error, etc... Safeguards...
NikoGrano Dec 11, 2018
e4e33f2
CS Fixer
NikoGrano Dec 11, 2018
9cd8d5d
Fixed failing tests.
NikoGrano Dec 11, 2018
2b5dbfd
Added clean Error Message in case of same class dependency
NikoGrano Dec 12, 2018
6b37bd3
Updated Test for new Skip message in case of same class test failing …
NikoGrano Dec 12, 2018
e53378d
Implemented tests for PR 3349
NikoGrano Dec 12, 2018
2f58830
CS Fixer
NikoGrano Dec 12, 2018
0d01999
Added more tests
NikoGrano Dec 12, 2018
7a95ba9
Fixed tests
NikoGrano Dec 12, 2018
ef8ec3e
Never commit when in underground and hurry to get out on next stop. F…
NikoGrano Dec 12, 2018
845d689
Removed cache file
NikoGrano Dec 13, 2018
58d289d
Implemented more tests
NikoGrano Dec 13, 2018
c33a58a
CS Fixer
NikoGrano Dec 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
105 changes: 78 additions & 27 deletions src/Framework/TestCase.php
View file
Open in desktop
Expand Up @@ -1685,10 +1685,12 @@ private function verifyMockObjects(): void
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);
$className = \get_class($this);
$passed = $this->result->passed();
$failed = $this->result->failures();
$skipped = $this->result->skipped();
$passedKeys = \array_keys($passed);
$numKeys = \count($passedKeys);

for ($i = 0; $i < $numKeys; $i++) {
$pos = \strpos($passedKeys[$i], ' with data set');
Expand All @@ -1698,7 +1700,33 @@ private function handleDependencies(): bool
}
}

$passedKeys = \array_flip(\array_unique($passedKeys));
$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:

phpunit/src/Runner/TestSuiteSorter.php

Lines 77 to 94 in b8f38d3

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

phpunit/src/Framework/TestCase.php

Lines 1685 to 1780 in b8f38d3

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;
}

phpunit/src/Framework/TestCase.php

Lines 1097 to 1116 in b8f38d3

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.


if ($pos !== false) {
$failedKeys[] = \substr($failure->getTestName(), 0, $pos);
} else {
$failedKeys[] = $failure->getTestName();
}
}

$skippedKeys = [];

foreach ($skipped as $skip) {
$pos = \strpos($skip->getTestName(), ' with data set');

if ($pos !== false) {
$skippedKeys[] = \substr($skip->getTestName(), 0, $pos);
} else {
$skippedKeys[] = $skip->getTestName();
}
}

$passedKeys = \array_flip(\array_unique($passedKeys));
$failedKeys = \array_flip(\array_unique($failedKeys));
$skippedKeys = \array_flip(\array_unique($skippedKeys));

foreach ($this->dependencies as $dependency) {
$deepClone = false;
Expand All @@ -1724,31 +1752,54 @@ private function handleDependencies(): bool
$dependency = $className . '::' . $dependency;
}

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

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
);
if (
!isset($skippedKeys[$dependency])
&& !isset($failedKeys[$dependency])
&& \strpos($dependency, '::') > 0
&& $className !== \explode('::', $dependency, 2)[0]
) {
$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 •
edited

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..

} else {
$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);

$this->result->endTest($this, 0);
return false;
}
}

return false;
$dependencyName = false;
if (isset($passed[$dependency]))
{
$dependencyName = $dependency;
} else if (isset($passed[$dependency.$this->getDataSetAsString(false)]))
{
$dependencyName = $dependency.$this->getDataSetAsString(false);
}

if (isset($passed[$dependency])) {
if ($passed[$dependency]['size'] != \PHPUnit\Util\Test::UNKNOWN &&
if ($dependencyName) {
if ($passed[$dependencyName]['size'] != \PHPUnit\Util\Test::UNKNOWN &&
$this->getSize() != \PHPUnit\Util\Test::UNKNOWN &&
$passed[$dependency]['size'] > $this->getSize()) {
$passed[$dependencyName]['size'] > $this->getSize()) {
$this->result->addError(
$this,
new SkippedTestError(
Expand All @@ -1764,11 +1815,11 @@ private function handleDependencies(): bool
$deepCopy = new DeepCopy;
$deepCopy->skipUncloneable(false);

$this->dependencyInput[$dependency] = $deepCopy->copy($passed[$dependency]['result']);
$this->dependencyInput[$dependency] = $deepCopy->copy($passed[$dependencyName]['result']);
} elseif ($shallowClone) {
$this->dependencyInput[$dependency] = clone $passed[$dependency]['result'];
$this->dependencyInput[$dependency] = clone $passed[$dependencyName]['result'];
} else {
$this->dependencyInput[$dependency] = $passed[$dependency]['result'];
$this->dependencyInput[$dependency] = $passed[$dependencyName]['result'];
}
} else {
$this->dependencyInput[$dependency] = null;
Expand Down