Replace xpath() with WebTestBase::cssSelect() by leveraging Symfony CssSelector

Looks nice and actually more usable for newbies to use

Seems reasonable to me. My only question would be how much effort we continue to put into Simpletest/WebTest in the first place. :-) I guess we can't get rid of it entirely any time soon, though.

Web tests via PHPUnit/Mink/Goutte are most likely not going to happen for D8 (since that is a completely different can of worms), so keep on rocking there.

xpost

+++ b/core/vendor/composer/autoload_classmap.phpundefined
@@ -3,350 +3,350 @@
-    'PHP_Token_XOR_EQUAL' => $baseDir . '/vendor/phpunit/php-token-stream/PHP/Token.php',
-    'SessionHandlerInterface' => $baseDir . '/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Resources/stubs/SessionHandlerInterface.php',
-    'Text_Template' => $baseDir . '/vendor/phpunit/php-text-template/Text/Template.php',
+    'File_Iterator' => $baseDir . '/core/vendor/phpunit/php-file-iterator/File/Iterator.php',
+    'File_Iterator_Facade' => $baseDir . '/core/vendor/phpunit/php-file-iterator/File/Iterator/Facade.php',

These changes are just from the composer.json move. Nothing to worry about. PHPUnit tests still run fine.

$ git apply css-selector-1959660.13.patch
css-selector-1959660.13.patch:1080: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

That's the .gitignore from CSS Selector. No problem.

Good catch on the docblock.

Since this adds a new library, assigning to Dries. TBH, despite the noted improvement on test readability, this kinda strikes me as "feature-ish" and should probably be subject to thresholds. Not touching the status for now.

64 files changed, 4906 insertions, 394 deletions.

That's a, um. Serious diffstat. :P

This could use a more fleshed-out issue summary to help understand why this is necessary (some before/after snippets — oops, that's there already, didn't scroll down enough :D), what other libraries were considered, how stable this library is in terms of Symfony's LTS support, etc.

It includes a lot of new files because it adds the Symfony CSS Selector.

Attached is the above patch without running composer update symfony/css-selector, or the included vendor code. You can see it replaces the ugly XPath selectors with nicer, much more familiar, CSS selectors. Makes it much easier to write tests which check output of pages.

3 files changed, 29 insertions, 37 deletions.

Well, whether it's pulled in via composer or committed directly to the vendor branch, it's still an extra 4600 lines of code we're loading for doing this task that we didn't load before.

Great idea! This would make my life so much easier.

this is awesome!
how about adding it to UnitTestCase as well?

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -1848,6 +1848,41 @@ protected function xpath($xpath, array $arguments = array()) {
+  protected function cssSelect($selector, $contains = TRUE, $content = FALSE) {

It would be so great to actually move this to TestBase so also drupal unit tests could use it. Just for example when testing the output of a table in views, I don't give a shit about the whole page, as I can render a view properly in the test.

Both me and dawehner think that testBase should be a better place. you dont need to be in a webtest to test X/Html output

Moved it and tried to write a test, but it feels like the DX is not perfect.

#32: drupal-1959660-32.patch queued for re-testing.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestUnitTest.phpundefined
@@ -0,0 +1,44 @@
+    debug($this->cssSelect('#test_id', TRUE, $content));

shouldnt this be an assertion isntead of debug?

Dreditor doesnt seem to like this patch...

The only major issue I could see was the debug() statement. The DX is fine, IMHO.

Edit: duplicate comment

I just created a duplicate #2030369: Add Symfony CssSelector to core which was needed for #2028535: Provide a TourTestBase class for use by core and contrib modules where the CSS selections come from Tour YAML files which we never can code against.

So I really hope this gets in :)

Patch needed a reroll. Had to rewrite one Tour test.

I've fixed #34 and added some more unit tests to core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestUnitTest.php

What I liked about using #2030369: Add Symfony CssSelector to core instead it's selector is a one shot instead of make sure to address the right contexts on complex css selection. But it's a HUGE improvement over XPath selection.

This needs tests for the comment from the function documentation.

* @param string $contains
* (optional) Filter matching nodes to those containing the given content.
* Note that due to limitations with \PHPUnit_Util_XML::cssSelect this does
* not work when you pass a nested selector such as 'li h2'.

We need a tests for ie

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestUnitTest.php
@@ -0,0 +1,73 @@
+    $child = $this->cssSelect('div span p', TRUE, $content);

Make a failing one.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestUnitTest.phpundefined
@@ -0,0 +1,73 @@
+class SimpleTestUnitTest extends UnitTestBase {

We wrote other phpunit tests for simpletest before, so why not use it here as well? I guess B => C would be enough

This isn't ready for a change notification until it goes into HEAD.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1344,4 +1344,42 @@
+   * @param mixed $content
+   *   (optional) Context to parse for selector.

"Context" should be "Content".

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1344,4 +1344,42 @@
+    if (!$content && isset($this->content)) {
+      $content = $this->content;

It would be nice to have some tests that extend WebTestBase so that we can test the usage of $this->content. Granted, the Tour tests already do this, but developers looking for some example implementations will perhaps not go looking in there.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.phpundefined
@@ -1344,4 +1344,42 @@
+    if (is_array($content)) {
+      $content = array_reduce($content, function ($value, $item) {
+        $value .= $item->ownerDocument->saveHtml($item);
+        return $value;
+      }, '');

To properly test the anonymous function inside the array_reduce() I would like to add a test that takes a content array with multiple items, in addition to the tests using an array with a single item.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestUnitTest.phpundefined
@@ -0,0 +1,73 @@
+  public static function getInfo() {
+    return array(
+      'name' => 'SimpleTest functionality (Unit test)',
+      'description' => "Tests some methods of Simpletest as unit test.",
+      'group' => 'SimpleTest'
+    );

Maybe shorten the name of the unit test from "SimpleTest functionality (Unit test)" to "SimpleTest unit tests" to be consistent with most other unit tests.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/SimpleTestUnitTest.phpundefined
@@ -0,0 +1,73 @@
+    $root = $this->cssSelect('#test_id', TRUE, $content);
+    $this->assertEqual(count($root), 1, 'ID selector works');
+
+    $child = $this->cssSelect('p', 'My Subchild My Child', $content);
+    $this->assertEqual(count($child), 1, 'Text content found.');

I think $elements would be a better name for the variable that holds the selected elements (instead of $root or $child).

My new hobby is to do code coverage analysis with XDebug, and I'm happy to announce that the tests cover 100% of the code. To be fully complete though, we should ensure that an empty array is returned when no match was found.

Regarding comments #39 and #40:

This needs tests for the comment from the function documentation.

* @param string $contains
* (optional) Filter matching nodes to those containing the given content.
* Note that due to limitations with \PHPUnit_Util_XML::cssSelect this does
* not work when you pass a nested selector such as 'li h2'.

I too wonder where this is coming from. @dawehner, how did you figure this out? I couldn't find anything about this limitation in the documentation of PHPUnit. I think this might just be a bug. If so, we should either find an existing bug report, or create one, and refer to that. Also if it is a bug in PHPUnit we should not add a test for that, that should be added in PHPUnit.

We wrote other phpunit tests for simpletest before, so why not use it here as well? I guess B => C would be enough

@dawehner, I don't catch your drift, what do you mean with B => C?

Attached patch addresses my own remarks, #39 and #40 are not yet addressed.

Marked #597866: Support for QueryPath-based assertions as a duplicate of this issue.

* @param string $contains
* (optional) Filter matching nodes to those containing the given content.
* Note that due to limitations with \PHPUnit_Util_XML::cssSelect this does
* not work when you pass a nested selector such as 'li h2'.

This was first mentioned by larowlan in comment #27. Indeed, PHPUnit_Util_XML::cssSelect() is very simple in comparison to the one from Symfony, and doesn't support this. It is still better than nothing though, and certainly easier than XPath.

I don't think we need to test for this case if it is simply not supported upstream.

Added the familiar [contains(concat(" ", normalize-space(@class), " ")] XPath drama to the example.

42: drupal-1959660-42.patch queued for re-testing.

1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -1326,4 +1326,45 @@ public function copyConfig(StorageInterface $source_storage, StorageInterface $t
   +   *   TRUE will match all content. Note that due to limitations with
   +   *   \PHPUnit_Util_XML::cssSelect this does not work when you pass a nested
   +   *   selector such as 'li h2'.
   

   That is a pretty heavy limitation. It degrades the very DX that we're trying to fix here back to a similarly awkward level as XPath. :-(

   Does the Symfony CssSelector component also have that limitation? — If it does not, then I'd actually rather go back to the original proposal to simply include that.

   Otherwise, it appears that some of the converted tests are working around that limitation by recursively calling cssSelect()... — so if pulling in the Symfony CssSelector component is not an option, I wonder whether we could at least automate that to retain a sane DX?

   CSS selectors are predictable and standardized, so something along the lines of the following might work:

   if (isset($contains) && !isset($content) && 1 < $nested = explode($selector, ' ')) {
     $content = $this->cssSelect($selector);
     $selector = end($nested);
   }
   

2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -1326,4 +1326,45 @@ public function copyConfig(StorageInterface $source_storage, StorageInterface $t
   +  protected function cssSelect($selector, $contains = TRUE, $content = FALSE) {
   

   The signature looks really strange to me.

   $selector is fine.

   But why does $contains default to TRUE? The default should be NULL.

   It's also not 100% clear to me what exactly $contains means — (1) does it operate on the raw content/markup (including elements), or (2) does it operate on the raw HTML node values, and in any case, (3) does it operate on the raw HTML content or on decoded text? (cf. htmlentitydecode())

   Likewise, $content should default to NULL. — All of the Boolean flags in this method signature do not make sense.

   Let's also be more accurate in the phpDoc @params and @return data types; i.e.:

   @param string $contains
   @param string|\DOMElement[] $content

   @return \DOMElement[]

   Lastly, DOMElement or DOMNode? Not the same.

3. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -1326,4 +1326,45 @@ public function copyConfig(StorageInterface $source_storage, StorageInterface $t
   +        $value .= $item->ownerDocument->saveHtml($item);
   

   Note that "HTML" is all-uppercase in DOMDocument::saveHTML()

4. +++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
   @@ -1326,4 +1326,45 @@ public function copyConfig(StorageInterface $source_storage, StorageInterface $t
   +    if ($nodes = \PHPUnit_Util_XML::cssSelect($selector, $contains, $content, TRUE)) {
   
   +++ b/core/modules/simpletest/lib/Drupal/simpletest/Tests/CssSelectWebTestBaseTest.php
   @@ -0,0 +1,71 @@
   +  function testCssSelect() {
   +    // Construct a simple HTML page with a title and a nested menu.
   +    $testpage = <<<HTML
   

   The "XML" part in this method concerns me.

   Is this compatible with HTML5?

   Let's change the test to additionally operate on some HTML5 elements; i.e.:

   1. Simply wrap the list into a SECTION, followed by an ASIDE, and lastly a NAV.

   2. Add some DETAILS + SUMMARY markup.

   3. Add one or more incomplete/auto-closed tags; e.g., a paragraph that isn't terminated.

   4. Some more dummy elements, time/audio/video/etc.

Also I'd like to go on record again that we are using composer wrong, we shouldn't include core/vendor in version control, just composer.lock.

I second the motion.

Thank you!

+++ b/composer.json
@@ -18,12 +19,12 @@
-    "guzzle/http": "3.7.*",
+    "guzzle/http": "3.7.1",
...
-    "zendframework/zend-feed": "2.2.*"
+    "zendframework/zend-feed": "2.2.1"

Seems unrelated? Can we revert these?

Thanks!

Next to the major argument of lacking HTML5 support, today I learned that the Behat/Mink framework also uses Symfony CssSelector — the grand master plan is to fully migrate from Simpletest to Mink (give or take Behat) in the long run (rather unlikely for 8.0.0, but definitely beyond).

So that makes two important reasons for going with Symfony CssSelector instead of PHPUnit_Util_XML.

Back to RTBC from me if green.

...aaaand back to RTBC

Thanks a lot, @larowlan!

This is desperately needed. XPath continues to cause pain for developers who are willing to write tests, but who get stuck on more complex DOM element assertions.

Let's pretty please end this pain right here and right now for D8. Sans the vendor component, the actual changes of this patch are almost neutral.

Tons of win at almost no cost. The one and only way to make everyone adopt the "tests by design" mantra is a stellar test authoring DX. This is a major contribution towards that goal.

I like the css selector stuff using mink/behat, good to introduce that here.

Needs Dries for the new library though.

Seems like a handy library to me. Green light to use it!

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

Committed/pushed to 8.x, thanks!

Yay!!

Change record looks good, does not need more explanation I think. I added a link to the documentation of the Symfony CssSelector component for people that like to read more.