PHPUnit deprecated assertTag(), assertNotTag() and assertSelectEquals()

I guess you could replace it with using symfony css selector + xpath without such a big effort.

Is it an idea to copy the functions assertTag() and assertSelectEquals() to the class UnitTestCase. Or replace the functions with an other assert call?

The development of phpunit-dom-assertions has stalled.

Change the issue to a bug report. Because it is stalling #2387027: Upgrade PHPUnit to the latest stable release.

Actually I'm going to go ahead and bump this to critical.

Leaving #2387027: Upgrade PHPUnit to the latest stable release as major since I'm not clear whether the update counts as a minor or major upgrade. Not being able to upgrade PHPUnit at all due to a BC break feels critical though.

If there's a backwards compatible way to fix this, or if we agree that the API change here would be OK in a minor release, we can bump this back down to major. Really don't want to end up with a forked/outdated PHPUnit for the entire lifecycle of 8.x though.

We could leverage https://github.com/phpunit/phpunit-dom-assertions

Given that we have our own base class I went with adding a PR for adding a nice test trait: https://github.com/phpunit/phpunit-dom-assertions/pull/4
but this PR is not a strict requirement to use that library.

Added a proposed solution to the IS.

Here we go.

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -17,7 +17,7 @@
-abstract class UnitTestCase extends \PHPUnit_Framework_TestCase {
+abstract class UnitTestCase extends \PHPUnit_Framework_DOMTestCase {

Ideally not all our unit tests would extend from dom test, but well, maybe we have to wait until the trait is in.
Does someone wants to create an issue which moves all our helper methods of the base test into some helper trait?

test runs fine locally with the patch

@jibran
Ensure to proxy the phpunit tests via simpletest. It looks like the autoloading doesn't work somehow.

composer dump-autoload was the sauce

+++ b/composer.json
@@ -28,7 +28,8 @@
+    "phpunit/phpunit-dom-assertions": "1.0.*@dev"

Can we ask them to tag a stable release?

There is no stable release yet.

Please see https://github.com/phpunit/phpunit-dom-assertions/issues/3

So should we pin a particular commit in the meantime?

Well last commit was on Aug 27, 2014. So I think it doesn't make much difference.

Also #2304461: KernelTestBaseTNG™ and #2232861: Create BrowserTestBase for web-testing on top of Mink are related here.

Does someone wants to create an issue which moves all our helper methods of the base test into some helper trait?

Here we go #2403087: Use phpunit-dom-assertions trait to allow simple extensibility of existing test classes

PS: Self quoting is not good. :P

Can we not rewrite the tests to use xpath processor. In #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 we are adding one for HTML5. If I am wrong about this please tell me.

@daffie
We talk about a phpunit test here, all that xpath functionality is just included in the simpletest side of things, afaik.

So should we pin a particular commit in the meantime?

I would still argue that we should point to a specific commit, it is the good practise, people read the composer.json file of core.

Here we go.

Thank you!

I think the latest stable dom crawler is 2.6.3 but this patch is coming with 2.6.1

Added "symfony/dom-crawler": "2.6.*", to drupal composer.

Can we use the more flexible ~2.6 or failing that ^2.6.3 if ~2.6 doesn't force 2.6.3?

+++ b/composer.json
@@ -17,6 +17,7 @@
     "symfony/validator": "2.6.*",
     "symfony/process": "2.6.*",
     "symfony/yaml": "2.6.*",
+    "symfony/dom-crawler": "2.6.*",

This is what we are doing with all the symfony components so it only seems fair to add 2.6.*

Ok, thanks for that

The only thing in this patch being added is the symfony/dom-crawler library. I cannot see why that is enough to fix the problem as stated in the issue summary.
In #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 is spoken about the symfony/dom-crawler library and the consensus is that an other library is far better then this one.
The problem with the symfony/dom-crawler library is that it only supports html4 and Drupal 8 wants html5 support.
So if there is a need for a dom-crawler library I am postponing this issue on #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5.

@daffie we are not adding symfony/dom-crawler we are adding phpunit/phpunit-dom-assertions. This is a critical issue if you have any other suggestion to fix this please explain.

This would need a re-roll now that #1975220: Allow a Composer user to manage Drupal, modules, and PHP dependencies with a custom root composer.json is in?

reroll

Thanks for the reroll @cilefen. RTBC anyone?

Lets do it.

1. +++ b/core/composer.json
   @@ -17,6 +17,7 @@
   +    "symfony/dom-crawler": "2.6.*",
   

   @daffie we are not adding symfony/dom-crawler

   Yes, you do!!! I can also see that is a requirement for the phpunit/phpunit-dom-assertions library.

2. But is it necessary to add the symfony/dom-crawler library to the core/composer.json file? Won't it be included with the core/vendor/phpunit/phpunit-dom-assertions/composer.json file?
3. The phpunit/phpunit-dom-assertions library still does not support the assertTag functionality. So the problem as described in the issue summary is not solved. For that I change the status to "needs work".

Yes, you do!!! I can also see that is a requirement for the phpunit/phpunit-dom-assertions library.

So? The tests for HTML are fine with just HTML 4 support, if we need HTML 5 support we can still switch over later.
This issue allows us to update phpunit in the meantime.

This issue won't be properly resolved until phpunit/phpunit-dom-assertions supports assertTag() and the other methods marked as deprecated, since without that, our tests that use those assertions fail when running phpunit 4.2+ (the failure is because of the deprecated flag).

assertTag() is only used in \Drupal\Tests\Core\Utility\LinkGeneratorTest. Isn't it a good plan to replace them with another approach so this can move forward?

assertSelectEquals() is also only used in one test: \Drupal\Tests\Core\Template\AttributeTest. If we can replace these instances we can update to the new PHPUnit and do not need to add a dependency on phpunit/phpunit-dom-assertions.

Patch overrides the assertTag() method that is used in LinkGeneratorTest with an alternative implementation that uses PHP built-in DOMDocument and DOMXPath classes to assert the tags.

If we combine this with the patch from #40 then we already do not need to wait for phpunit/phpunit-dom-assertions to add support for assertTag() before moving on with this issue. I would also propose to investigate whether we can replace assertSelectEquals() with an alternative solution.

Setting to needs review for the bot. This still needs work.

This patch replaces all occurrences of assertTag() and assertSelectEquals() with self rolled implementations. This should make it unnecessary to include phpunit/phpunit-dom-assertions.

Please review carefully. I am very drunk.

1. +++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
   @@ -331,16 +331,88 @@ public function testPrint() {
   +    $document->preserveWhiteSpace = FALSE;
   

   Would be useful if there is some documentation why preserveWhiteSpace needs to be set to FALSE.

2. +++ b/core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
   @@ -445,11 +445,44 @@ public function testGenerateActive() {
   +    // The parent method is deprecated in PHPUnit 4.2. This is an alternative
   

   if it is deprecated, should we still use inheritdoc? If the method will disappear, so will the documentation.

Ok let's try it out for real. Attached patches update PHPUnit to 4.2.6. The first is only the update, without patch. This one should fail with deprecated messages. The second one includes the patch from #50 and should hopefully be green.

If this goes through we can make a followup issue to update PHPUnit itself.

@Nick_vh, thanks for the review! I'll address your remarks and will post an updated patch. In the meanwhile I'll set the status back to needs review temporarily so we can get feedback from the bot.

Ok so we need to replace assertNotTag() as well.

Addressed the remarks from #51.

1. The preserveWhiteSpace() is not actually needed, so I removed it.
2. Good point. I renamed the method to assertLink() since we are only testing links here, not tags in general. This also allowed to simplify the calling code a bit.

I also implemented an alternative for the 2 calls to assertNotTag().

2 patches attached: first is the vanilla patch, the second one includes the update of PHPUnit to 4.2.6 to prove that it actually works with the new version.

Another experiment, PHPUnit 4.2.x is also quite old already. Let's see how our test suite fares using the patch and the very latest PHPUnit 4.4.2.

The actual patch to review is the first patch from #57.

Awesome work @pfrenssen. Why not move these new function to UnitTestBase?

Wow all tests green on PHPUnit 4.4.2! That is great news, I always had understood that it would be very hard to update PHPUnit due to their lack of backwards compatibility.

I have created a followup issue to do this: #2409667: Update to PHPUnit 4.4.2.

@jibran, I think these tests methods are too narrow in scope to be of general use. It's true we have a need for parsing HTML, but #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 seems like the ideal solution for this. I'd rather get that in than maintaining our own set of DOMDocument based parsing in UnitTestBase.

Ok then. From #50 onward issue completely changed directions so I think I can RTBC this. Thanks @pfrenssen for the explanation.

@pfrenssen you created #2409667: Update to PHPUnit 4.4.2 but we do have #2387027: Upgrade PHPUnit to the latest stable release

Ohh I had been searching the issue queue extensively before creating the issue, but I was using the keyword "update" instead of "upgrade".

Thanks for the RTBC!

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 3e43de7 and pushed to 8.0.x. Thanks!