-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Implement filtering tests based on XML input file #4449
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4449 +/- ##
============================================
+ Coverage 83.54% 83.63% +0.09%
- Complexity 4554 4578 +24
============================================
Files 286 287 +1
Lines 11759 11820 +61
============================================
+ Hits 9824 9886 +62
+ Misses 1935 1934 -1
Continue to review full report at Codecov.
|
|
||
// Regular PHPUnit tests | ||
foreach ($xml->testCaseClass as $class) { | ||
$className = $class['name']->__toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this typesafe? Maybe through an annotation like this:
/** @var array{name: ?} $class */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the feedback!
I'll first wait regarding such typings as to in what direction the PR really goes.
I never used SimpleXml really before, this is all highly magic. $class
is not an array
, it's SimpleXMLElement
providing lots-o-magic stuff…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the time you invest in preparing this pull request. Please note, though, that I will not accept this if it introduces ext/simplexml
usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've replaced SimpleXML with DomDocument and re-used the existing \PHPUnit\Util\Xml\Loader::loadFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great @mfn, looks pretty good overall and opens the door for some very important functionality.
f594394
to
b10d64a
Compare
Can we please stop using none namespaced XML and provide XSDs for them? |
ping, anyone? |
Not sure what you're expecting at this moment. While the current state of this PR is having conflicts that should get resolved, my comment regarding the lack of a namespace and an XSD is unanswered. I do realize you're only relying on the export that already is broken in that regard. But before building more things that rely on this, we should fix that, imho. |
@mfn I am working on writing a more detailed review. Long story very short: the idea of filtering tests via an XML-file and the main Iterator is fine, but it needs refactoring. I am also wary of any iterator work getting in the way of the coming redesign of the core iterator/filtering mechanisms. If this PR gets accepted as-is, I would have to refactor this new functionality, too. |
} | ||
|
||
/** @var TestCase $test */ | ||
$testClass = get_class($test); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a way to ask anything that implements a Test
its own unique identity, have a look at https://github.com/sebastianbergmann/phpunit/blob/master/src/Framework/Reorderable.php and the uses of Reorderable
.
If you need more details about the (origins of) a test, let me know the use cases. It would be best if we can extend a central mechanism for identifying and locating tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to concentrate on this part first here before solving the others, might have an impact.
When a filter receives a test class in accept()
, I've to figure out ifs part of the desired filter, i.e. the XML being feed back to phpunit.
The format of the XML is defined via \PHPUnit\Util\XmlTestListRenderer::render
phpunit/src/Util/XmlTestListRenderer.php
Line 42 in 1766543
if (get_class($test) !== $currentTestCase) { |
and creates a XML structure like this:
<?xml version="1.0"?>
<tests>
<testCaseClass name="PHPUnit\Framework\FunctionsTest">
<testCaseMethod name="testGlobalFunctionsFileContainsAllStaticAssertions" groups="default" dataSet=""assertArrayHasKey""/>
…
</testCaseClass>
<phptFile path="/absolute/path/phpunit/tests/end-to-end/abstract-test-class.phpt"/>
…
</tests>
And this now explains why I'm using get_class()
and not something else: as a consumer of the XML, I've to match the producer.
This is, for \PHPUnit\Framework\TestCase
, further stipulated with the dataSet
-attribute, it's produced using this code
phpunit/src/Util/XmlTestListRenderer.php
Lines 57 to 66 in 1766543
if (!empty($test->getDataSetAsString(false))) { | |
$writer->writeAttribute( | |
'dataSet', | |
str_replace( | |
' with data set ', | |
'', | |
$test->getDataSetAsString(false) | |
) | |
); | |
} |
As can be seen, this manually removes some parts of \PHPUnit\Framework\TestCase::getDataSetAsString
and writes it to the XML.
For that reason, when reading the XML I'm reconstructing the original "data as string" with this:
$name = "{$methodName} with data set {$dataSet}";
$this->filter[$className][$name] = true;
so that in accept()
I can just call
$name = $test->getName();
and getName
internally calls getDataSetAsString
, so that in the end by calling:
$testClass = get_class($test);
inaccept()
and doing"{$methodName} with data set {$dataSet}
insetFilter
I've built the matching mirror logic for consuming what was produced.
I guess some of this "peekaboo" here is reflected in #4449 (comment)
So currently I don't see how e.g \PHPUnit\Framework\TestCase::sortId
(of \PHPUnit\Framework\Reorderable
) helps me here, as the generated value is not usable in the context of what is produced in the XML
As for the format used in the \PHPUnit\Runner\Filter\XmlTestsIterator::$filter
, I tried to document it:
/**
* The filter is used as a fast look for
* - the class name
* - the method name plus its optional data set description.
*
* Example: `filter[class name][method name + data set] = true;`
*
* The accept() method then can use a fast isset() to check if a test should
* be included or not.
*
* This works equally for phpt tests, except we hardcode the class name.
*
* @var array<string,array<string,true>>
*/
*/ | ||
private function setFilter(string $xmlFile): void | ||
{ | ||
$xml = (new XmlLoader())->loadFile($xmlFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the iterator gets instantiated multiple times, this becomes a very expensive implementation. Loading the configuration should be done once, stored somewhere and then used as a lookup in the iterator instances.
Currently this would be somewhere around TestRunner::processSuiteFilters()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I took a closer look and finally figured out why processSuiteFilters
is only executed once but the constructor/filter called multiple times => I didn't realize this whole reflection/instantiation the first time around.
I'm not sure where exactly to put this code to a) load the XML b) parse it c) generate the data structure for fast lookup, so to satisfy the performance concerns for now, I left the code within XmlTestsIterator
as static methods and call this helpers from processSuiteFilters
.
What I find "nice" about this is that generating the filter and consuming it in accept()
is in the same code space; I kinda like to think this makes easier to understand.
I'm happy to move it somewhere else, but wasn't sure if putting them directly into \PHPUnit\TextUI\TestRunner
would be a good idea either.
private function extractTestCases(DOMXPath $xpath): Generator | ||
{ | ||
/** @var DOMElement $class */ | ||
foreach ($xpath->evaluate('/tests/testCaseClass') as $class) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as loading the XML-file for every new Iterator: please do this once and use a centralized lookup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now addressed, see response to https://github.com/sebastianbergmann/phpunit/pull/4449/files#r503837936
{ | ||
/* @var DOMElement $phptFile */ | ||
foreach ($xpath->evaluate('/tests/phptFile') as $phptFile) { | ||
$path = $phptFile->getAttribute('path'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why does CodeCov mark these lines as not-reached?
$path = $phptFile->getAttribute('path'); | ||
|
||
if ($path) { | ||
yield $path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why does CodeCov mark these lines as not-reached?
@@ -1145,6 +1147,13 @@ private function processSuiteFilters(TestSuite $suite, array $arguments): void | |||
); | |||
} | |||
|
|||
if (!empty($arguments['testsXml'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Around here is the place to load+parse your configuration and get a list of tests. Perhaps you can even adapt/reuse the current NameFilterIterator
, as it's basically what it is. :)
5d5ef3e
to
4303365
Compare
For some reason I never got the email notification from the review feedback #4449 (review) , I just discovered it now 💥 I'll try to find time ASAP to address things! |
6860ef8
to
fdc3f6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epdenouden thanks for your feedback!
I did explain some specifics regarding the accept()
implementation, as I'm not sure what I could do to improve it regards "interoperability" with consumed XML.
Technically I did address the performance concern regarding the XML parsing, though I'm not yet sure if the code location is fine or not.
I'll address code coverage feedback once we all agree what the final implementation is supposed to look like to move forward.
Thank you!
} | ||
|
||
/** @var TestCase $test */ | ||
$testClass = get_class($test); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to concentrate on this part first here before solving the others, might have an impact.
When a filter receives a test class in accept()
, I've to figure out ifs part of the desired filter, i.e. the XML being feed back to phpunit.
The format of the XML is defined via \PHPUnit\Util\XmlTestListRenderer::render
phpunit/src/Util/XmlTestListRenderer.php
Line 42 in 1766543
if (get_class($test) !== $currentTestCase) { |
and creates a XML structure like this:
<?xml version="1.0"?>
<tests>
<testCaseClass name="PHPUnit\Framework\FunctionsTest">
<testCaseMethod name="testGlobalFunctionsFileContainsAllStaticAssertions" groups="default" dataSet=""assertArrayHasKey""/>
…
</testCaseClass>
<phptFile path="/absolute/path/phpunit/tests/end-to-end/abstract-test-class.phpt"/>
…
</tests>
And this now explains why I'm using get_class()
and not something else: as a consumer of the XML, I've to match the producer.
This is, for \PHPUnit\Framework\TestCase
, further stipulated with the dataSet
-attribute, it's produced using this code
phpunit/src/Util/XmlTestListRenderer.php
Lines 57 to 66 in 1766543
if (!empty($test->getDataSetAsString(false))) { | |
$writer->writeAttribute( | |
'dataSet', | |
str_replace( | |
' with data set ', | |
'', | |
$test->getDataSetAsString(false) | |
) | |
); | |
} |
As can be seen, this manually removes some parts of \PHPUnit\Framework\TestCase::getDataSetAsString
and writes it to the XML.
For that reason, when reading the XML I'm reconstructing the original "data as string" with this:
$name = "{$methodName} with data set {$dataSet}";
$this->filter[$className][$name] = true;
so that in accept()
I can just call
$name = $test->getName();
and getName
internally calls getDataSetAsString
, so that in the end by calling:
$testClass = get_class($test);
inaccept()
and doing"{$methodName} with data set {$dataSet}
insetFilter
I've built the matching mirror logic for consuming what was produced.
I guess some of this "peekaboo" here is reflected in #4449 (comment)
So currently I don't see how e.g \PHPUnit\Framework\TestCase::sortId
(of \PHPUnit\Framework\Reorderable
) helps me here, as the generated value is not usable in the context of what is produced in the XML
As for the format used in the \PHPUnit\Runner\Filter\XmlTestsIterator::$filter
, I tried to document it:
/**
* The filter is used as a fast look for
* - the class name
* - the method name plus its optional data set description.
*
* Example: `filter[class name][method name + data set] = true;`
*
* The accept() method then can use a fast isset() to check if a test should
* be included or not.
*
* This works equally for phpt tests, except we hardcode the class name.
*
* @var array<string,array<string,true>>
*/
*/ | ||
private function setFilter(string $xmlFile): void | ||
{ | ||
$xml = (new XmlLoader())->loadFile($xmlFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I took a closer look and finally figured out why processSuiteFilters
is only executed once but the constructor/filter called multiple times => I didn't realize this whole reflection/instantiation the first time around.
I'm not sure where exactly to put this code to a) load the XML b) parse it c) generate the data structure for fast lookup, so to satisfy the performance concerns for now, I left the code within XmlTestsIterator
as static methods and call this helpers from processSuiteFilters
.
What I find "nice" about this is that generating the filter and consuming it in accept()
is in the same code space; I kinda like to think this makes easier to understand.
I'm happy to move it somewhere else, but wasn't sure if putting them directly into \PHPUnit\TextUI\TestRunner
would be a good idea either.
private function extractTestCases(DOMXPath $xpath): Generator | ||
{ | ||
/** @var DOMElement $class */ | ||
foreach ($xpath->evaluate('/tests/testCaseClass') as $class) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now addressed, see response to https://github.com/sebastianbergmann/phpunit/pull/4449/files#r503837936
Btw, is it fine to target master or should I target another branch, perhaps 9.5? |
Export via `--list-tests-xml` and cut and slice as wanted, to feed it back via `--tests-xml`.
Is there anything else I can do? AFAIK I addresses the feedback or did I miss anything, which blocks us? |
Let me up this ticket. Would like to help (if any help is needed) because this feature will greatly help us at Infection with filtering what tests to run (including data sets) |
I didn't look into the Code itself, as @epdenouden already did that and I trust he'll review the current state as well ;) But I'd like to repeat my statement made earlier: We should clean up the XML produced before we add more functionality and have (more) external projects rely on it. At the very least, let us please add a namespace with a version identifier so any future upgrades and changes are not going to kill us. |
I definitely feel caught in a catch-22 here and I get no clear indicator how I can move forward. I feels out of scope for me to tackle the "XML / namespace / schema" problem here, that's not a problem I created and seems "higher up" to me. Randomly I tried to update the PR and resolve the merge conflicts, but I got demotivated due to lack of feedback / specific actionable stuff, so I didn't touch it for some time. In any way, I'm about to leave for vacation and can't provide updates within the next few weeks. I'll check the PR feedback when I get back but I'm also happy if someone else steps up and assists here 🙏 |
6aa2fbc
to
078538a
Compare
08dc555
to
63591f3
Compare
Thank you for your contribution. I appreciate the time you invested in preparing this pull request. However, I have decided not to merge it. |
Summary
Export via
--list-tests-xml
and cut and slice as wanted, to feed it back via--tests-xml
. Works with PHPUnit tests as well as PHPT tests.A cheap slicing script is provided for now at https://gist.github.com/mfn/256636242cbe8a51252ce28181a6b074
Example:
Overview
--tests-xml <xml file>
argument to PHPUnit\PHPUnit\Runner\Filter\XmlTestsIterator
which parses the XML and builds an internal data structure to identify what tests toaccept()
I consider the state of the PR in a "functional state" and before finishing it I would seek feedback if I missed parts and guidance what kind of tests are best.
Considerations
The handling of using SimpleXML and parsing in\PHPUnit\Runner\Filter\XmlTestsIterator::setFilter
is pretty crude. I barely worked with XML and PHP in the last decade. I saw\PHPUnit\Util\Xml\Loader
but wasn't sure if it should be used, it was just added a few days ago. Also I liked the "simplicity" ofSimpleXml
to get this up and running.=> Replaced with ext-xml aka
DomDocument
and friends--filter-tests-xml
, seemed more apt. But I ran into a strange problem withsebastian/cli-parser
I didn't really understand so I just gave it a different to get the ball rollingError when using --filter-tests-xml as option name
TODO
=> a phpt test has been added
DomDocument
?=> Indeed, changed to DomDocument
Links
=> Specify a list of tests to run #3387