Convert web tests to browser tests for comment module

Making a more extensive pass for each test to see what I can get to work with minimal effort. Will post a patch soon with a detailed update.

Fixed some tests but then hit a wall, AssertLegacyTrait is not fully compatible on assertFieldByName and assertNoFieldByName. The old methods searched for multiple fields and the versions in AssertLegacyTrait only on 1. Separate issue created: #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions.

Also re-moved all the tests for a cleaner patch, it applies nicer in phpStorm now :)

Setting to review to get an idea on what the testbot thinks of it, this is in no way ready for a review :)

Update, lots more tests are working and validated not-working. Included some unrelated files (files from the issue this one is postponed on).

List of files:
ViewTestBase.php
AssertLegacyTrait.php
BrowserTestBaseTest.php
TestForm.php

Fixing postComment will fix several tests, hope I can continue tomorrow.

Update. All tests now checked, got some more tests working. Found some incorrectly defined test, hope it will run on the test-bot now so we can see if it agrees with my list.

Next up: fix postComment to get more tests to the working column.

Fixed incorrect Trait usage, fails should go down.

Testbot reported more succes than me locally. I somehow have trouble with the user authentication stuff and permission checks. Gonna have to figure out why that is some day..

Right, found an interesting thing on ContentTranslationUITestBase. The base class was already in core after the big chunk conversion (#2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits) but was actually never used in core. It didn't completely work yet either, making the CommentTranslationUITest fail.

New patch has some more passes, hoping actually that the test-bot will return only 1 fail: CommentNewIndicatorTest. Didn't convert that one yet as I still have to figure out an alternative to curlExec(). I'm not sure yet on wether this really needs to be a browser test or something else. Some help on that one would be appreciated.

Right, more fails but still closer. RssTest now fixed and other fails on the other tests. Progress!

Analyzed the error reports some more, placed CommentPagerTest.php out of scope because it needs a JavaScript test.

Discussed with lendude, placing CommentNewIndicatorTest.php out of scope. It will most likely need a conversion to a Javascript test.

CommentTranslationUITest now working with updated AssertLegacyTrait for checkboxes and other small fixes.

Only one left: CommentPreviewTest. Would like someone's opinion on this one. There is a $this->setRawContent($content); to test resubmitting a comment. Not sure yet on what to do with that one.. I'm tempted to place it out of scope because it might stall this issue too long. Will decide on that once the issues that this one is postponed on get committed.

in general $this->setRawContent() means 'Turn me into a kernel test'.

CommentPreviewTest and CommentCSSTest now working.

Every test in the comment module that can be converted to BrowserTestBase has been converted. Clean and extra's patch attached. Please review only the clean patch.

Bigger changes:
- Moved generatePermutations to AssertHelperTrait, seemed like a good place, it's a helper function after all.

Yeah one less issue.

And we're good to go! Well, after I fix the last conversion issue that is :) Working on that right now.

Last one should be fixed now. Updated the follow-ups with references or issues, that should be covered as well now.

+++ b/core/modules/comment/tests/src/Functional/CommentPreviewTest.php
@@ -105,16 +110,13 @@
-    // Reset the content of the page to simulate the browser's back button, and
-    // re-submit the form.
-    $this->setRawContent($content);
-    $this->drupalPostForm(NULL, [], 'Save');
+    // Go back and re-submit the form.
+    $this->getSession()->getDriver()->back();

This is quite amazing

+++ b/core/modules/comment/tests/src/Functional/CommentAnonymousTest.php
@@ -43,7 +43,7 @@ public function testAnonymous() {
-    $preview = (string) $this->cssSelect('.preview')[0]->asXML();
+    $preview = (string) $this->getSession()->getPage()->find('css', '.preview')->getHtml();

Why can't we use the existing cssSelect method which is out there? It is not even marked as deprecated on BrowserTestBase

Replace $this->getSession()->getPage()->find('css', ...) with $this->cssSelect(...).

Thanks @Jo Fitzgerald

Patch now fails because the field assertions in #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions have been reverted, postponing untill we figure out the way forward on that one.

#2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions has landed. Re-queued the patch in #33

Patch from #33 re-rolled. There are errors and test failures when I run the tests locally, but worth seeing what the testbots make of it.

Well, at least the patch does get applied now.

18:36:58 PHP Fatal error: Trait method generatePermutations has not been applied, because there are collisions with other trait methods on Drupal\simpletest\TestBase in /var/www/html/core/modules/simpletest/src/TestBase.php on line 25

I saw this loaclly. The function generatePermutations() has been added to /core/modules/simpletest/src/AssertHelperTrait.php but not removed from /core/tests/Drupal/Tests/Traits/Core/GeneratePermutationsTrait.php

I do not know the reason for duplicating generatePermutations() in /core/modules/simpletest/src/AssertHelperTrait.php. It is needed for CommentCSSTest.php, so in that class I have added

+use Drupal\Tests\Traits\Core\GeneratePermutationsTrait;

and

+  use GeneratePermutationsTrait;

and removed the addition in AssertHelperTrait.php.

This is the only change from #38 and the tests now run locally. There are some failures, but I wanted to get things done one at a time.

That's good because (a) the tests now run and we do not get the colision of Trait method generatePermutations(), and (b) the test failures are exactly the same as on my local testing.

I do not know if anything has changed in the implementation of $this->cssSelect() but it returns an array, so we need to add [0] to get the object from which to call the method ->find(). Maybe this was working in the patch in #31 and before, I don't know, but it does not seem to work now. Adding [0] solves this - see the interdiff.

The patch also fixes the coding standards errors that were introduced in the patch and reported in the test output (I have not fixed the coding standard errors which already exist in the files which have no other changes - that is out of scope for this issue)

One final fix - the core button text seems to have changed from "delete comments" to just "delete" so the test needs to be adjusted. This was supposed to be in the last patch, but I missed it.

All tests pass, plus no new coding standards violations introduced.

Just for info, I found where the "Delete comments" button got changed in core and why that caused us a problem. The patch is in #1986606-362: Convert the comments administration screen to a view comment #362 and the commit was made to 8.5.x and 8.4.x on 29 July.

/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
   public function getConfirmText() {
-    return $this->t('Delete comments');
+    return $this->t('Delete');
   }

The comment tests were also fixed in line with this:

/core/modules/comment/src/Tests/CommentAdminTest.php
-    $this->drupalPostForm(NULL, $edit, t('Delete comments'));
+    $this->drupalPostForm(NULL, [], t('Delete'));

plus equivalent changes in /modules/comment/src/Tests/CommentNonNodeTest.php and /modules/comment/src/Tests/CommentTestBase.php

However, the patch from #31 of our issue above left the existing CommentTestBase.php in place, and created a new file /modules/comment/tests/src/Functional/CommentTestBase.php. Perfectly fine, except this was based on the original before the core commit of 29 July, hence still had the old button text.

Nice work @jonathan1055

Just for info, I found where the "Delete comments" button got changed in core and why that caused us a problem. The patch is in #1986606-362: Convert the comments administration screen to a view comment #362 and the commit was made to 8.5.x and 8.4.x on 29 July.

That's fine and it was because we started using action plugins in #1986606: Convert the comments administration screen to a view

Patch looks good to me. Minimal code changes and green yay!

Here's a patch with the same eventual output, but using diff -M90% --find-copies to show that the two CommentTestBase files are copies of the original, highlighting the minor changes. Easier to read and understand (and smaller).

Hate to pull this back from RTBC (I agree that what we have so far is ready), but the scope of this is now a little bigger then it was when this started rolling.

The Views tests and Update tests can be converted here, and if there is some reason they can't be, we need something in the IS to indicate why.

The tests that still remain unconverted in /comment/src/Tests are:

• three that are listed as out-of-scope and have follow up issues linked in the IS
• two in /Update/ which are now covered by the patch in #2905627: Part-2: Convert UpdatePathTestBase to BrowserTestBase, so I have updated the IS with this
• three in /Views/ which I think are the ones you are referring to, that can now be converted

OK I will work on the /Views/ tests and see what I can do.

I have converted the final three Comment tests in /src/Tests/Views. The patch also (temporarily) adds the getAllOptions() method into AssertLegacyTrait just for testing on d.o. but this is actually covered by #2907485: Add getAllOptions() to AssertLegacyTrait. Thanks Lendude.

One thing which had me mystified was that the WizardTest was failing in my local testing, both before the conversion (via run-tests.sh) and after conversion (via phpUnit). The problem was in the line $this->drupalGet($node_comment->toUrl('edit-form')->toString()); which gets the comment edit form. The url is 'comment/1/edit'.

Method drupalGet() can accept for the first parameter a url object or a string. If it gets an object then the full url is calculated and the correct result is returned. If it gets a string, the expectation is that this will be the url starting from the drupal root directory, and then the 'http://localpath/path-to-drupal-root' is added in front. But using ->toString() on the url object, as was done in this test, adds the 'path-to-drupal-root', so that when drupalGet() also adds this we have two lots of path-to-drupal-root. Clearly this does not happen on d.o. testing because the tests pass, but I was getting failures with 404 page not found. The test site I am using is named 'drupal85dev' so the correct url is 'http://localpath/drupal85dev/comment/1/edit'. However, the test code as it stands was giving me 'http://localpath/drupal85dev/drupal85dev/comment/1/edit' which does not exist.

Removing the ->toString() means that a url object is passed to drupalGet() and this solved the problem. All tests pass locally, but it will be interesting to see if this also passes on d.o. testsbots.

Looks good to me can be RTBC after #2907485: Add getAllOptions() to AssertLegacyTrait.

Thanks jibran.

Regarding #52 I am still concerned as to why the existing WizardTest passed on d.o. testbots but not locally, as this caused me several hours of effort, which would be nice to avoid someone else also wasting. It would be helpful to know what the local testing set-up/config should have been such that the tests would have passed before my change. Could it be that the testbots run in a Drupal environment placed immediately at the localpath root, such that the additional directory name (which got added twice in my local testing) is empty, and thus would not cause a problem. It seems unlikely that this has not been discovered before now. Maybe I will have to raise an issue in the testing queue.

@jonathan1055
I'm quite sure (99.5%) that the testbots install drupal in a folder, so these kind of problems are detected. Feel free to try out https://www.drupal.org/node/2487065 ... which allows you to run tests locally, but honestly this documentation is a bit rough :(

Yeah pretty sure it's installed in a folder. I've seen (and written I fear) a fair number of patches where the basepath was hardcoded to '/' in some tests and they would pass locally and then fail on d.o

Thanks @dawehner and @Lendude. My problem was that existing tests which were passing on d.o. failed in my local testing (before any changes) so I had to work out why. I don't want to de-rail this issue, so can you suggest a better place to discuss this problem? It's not really a DrupalCI testbot issue, is it?

#2907485: Add getAllOptions() to AssertLegacyTrait is in. Patch needs a reroll.

I have chcked and the existing patch still applies, once the change to AssertLegacyTrait.php is entirely removed. So no code changes and no interdiff, just patch 52 without the last chunk.

+++ b/core/modules/comment/src/Tests/CommentTestBase.php
@@ -2,6 +2,8 @@
+@trigger_error('\Drupal\comment\Tests\CommentTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\comment\Functional\CommentTestBase', E_USER_DEPRECATED);

@@ -12,6 +14,9 @@
+ * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Tests\comment\Functional\CommentTestBase instead.

+++ b/core/modules/comment/src/Tests/Views/CommentTestBase.php
@@ -2,13 +2,17 @@
+@trigger_error('\Drupal\comment\Tests\Views\CommentTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\comment\Functional\Views\CommentTestBase', E_USER_DEPRECATED);
...
+ * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Tests\comment\Functional\Views\CommentTestBase instead.

The deprecations require a Change record and a @see/See to that CR. Should be some examples for a CR floating around in the other conversion issues.

OK, thanks. I have created a change record https://www.drupal.org/node/2908490 - I thought it would be OK to have one record for both of these, given they are in the same module?
I have also seen #2908391: Add a rule for expected format of @deprecated and @trigger_error() text so will follow that standard. Couple of questions:

1. Is it better to use @trigger_error(__NAMESPACE__ . 'CommentTestBase' ... instead of @trigger_error('\Drupal\comment\Tests\Views\CommentTestBase ...? I have seen both used recently
2. Can the deprecated class use the newly-moved class instead of having the full sourcecode duplicated?, and if so is that preferred?

I think one CR is fine, they are both deprecated for the same reason after all. The CR looks good to me.

1. I personally prefer the __NAMESPACE__ version because I find it easier to read and it very clearly points to the current file (no staring at two long namespaces and trying to see if they are the same), but I don't think there is a rule currently.
2. Since the classes can contain (or in the future have) WebTestBase or BrowserTestBase specific logic, I think copying the code is the only good way forward. That way we are free to make changes in the new class that depend on the underlying test engine without worrying that we break something in the old class.

1. Yes I prefer the __NAMESPACE__ version. Another benefit is that the code line is shorter. Which is helpful because it becomes longer again now that I have added 'See' and the CR url to the message (as per the deprecation standards)
2. Of course, yes it is better to give freedom to change the new without having to worry about the old.

The interdiff shows two other changes I have made:
3. In the deprecated src/Tests/Views/CommentTestBase.php I believe the statement "use Drupal\views\Tests\ViewTestBase;" should not be removed. if it is deleted you cannot test the deprecation triggers via phpUnit, because you get "Class 'Drupal\comment\Tests\Views\ViewTestBase' not found in comment/src/Tests/Views/CommentTestBase.php"
4. The one-line description of the class was "Tests the argument_comment_user_uid handler." which is clearly left over from a copy-pasting. I have changed it to "Provides setup and helper methods for comment views tests." to match the description in src/Tests/Views/CommentTestBase.php

Remove "Needs reroll" tag

All the feedback addressed after #53 so it's RTBC.

Updating issue credits to include Lendude for mentoring/reviews

1. +++ b/core/modules/comment/tests/src/Functional/CommentPreviewTest.php
   @@ -105,16 +110,13 @@ public function testCommentPreviewDuplicateSubmission() {
   -    // Reset the content of the page to simulate the browser's back button, and
   -    // re-submit the form.
   -    $this->setRawContent($content);
   -    $this->drupalPostForm(NULL, [], 'Save');
   +    // Go back and re-submit the form.
   +    $this->getSession()->getDriver()->back();
   

   <3 swoon

2. +++ b/core/modules/comment/tests/src/Functional/Views/CommentAdminTest.php
   @@ -18,13 +18,13 @@
   -  protected function setUp() {
   -    parent::setUp();
   +  protected function setUp($import_test_views = TRUE) {
   +    parent::setUp($import_test_views);
   

   Have we mixed up our test base here? i.e. we're extending from the generic CommentTestBase but we're using the $import_test_views for the one in the views namespace?

Yeah, #66.2 is not needed.

Thanks!

Updating credits to include @dawehner for a review that changed the shape of the patch.
Not crediting myself

This patch looks great!

<3 swoon

:D

1. +++ b/core/modules/comment/tests/src/Functional/CommentBookTest.php
   @@ -1,10 +1,11 @@
   +use Drupal\comment\Tests\CommentTestTrait;
   
   +++ b/core/modules/comment/tests/src/Functional/CommentLanguageTest.php
   @@ -1,18 +1,19 @@
   +use Drupal\comment\Tests\CommentTestTrait;
   
   +++ b/core/modules/comment/tests/src/Functional/CommentNonNodeTest.php
   @@ -1,16 +1,17 @@
   +use Drupal\comment\Tests\CommentTestTrait;
   

   We're moving all functional comment tests out of Drupal\comment\Tests. Shouldn't the trait also be moved then?

   Then these new use statements would also not be necessary.

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
   @@ -227,10 +227,10 @@ protected function doTestTranslationOverview() {
   -        $this->assertEqual($elements[0]->getText(), $entity->getTranslation($langcode)->label(), new FormattableMarkup('Label correctly shown for %language translation.', ['%language' => $langcode]));
   +        $this->assertEqual($elements[0]->getHtml(), $entity->getTranslation($langcode)->label(), new FormattableMarkup('Label correctly shown for %language translation.', ['%language' => $langcode]));
            $edit_path = $entity->url('edit-form', ['language' => $language]);
            $elements = $this->xpath('//table//ul[@class="dropbutton"]/li/a[@href=:href]', [':href' => $edit_path]);
   -        $this->assertEqual($elements[0]->getText(), t('Edit'), new FormattableMarkup('Edit link correct for %language translation.', ['%language' => $langcode]));
   +        $this->assertEqual($elements[0]->getHtml(), t('Edit'), new FormattableMarkup('Edit link correct for %language translation.', ['%language' => $langcode]));
   

   These changes are not in comment module. I think they were added accidentally?

Thanks mpdonadio for the reroll and for removing that change from content_translation

Regarding the CommentTestTrait with 80 usages, I would suggest we do that in a follow-up issue? This current issue has already suffered several does-not-apply-needs-reroll and adding that big list of extra moves will make it unworkable. Let's get this in first, as a lot of work has been done and is virtually ready to be RTBC.

#73: hah, good point — I totally missed those other usages! Definitely out of scope then.

Back to RTBC!

Updating credits to include WimLeers for review

Committed as 73bba85 and pushed to 8.5.x

Cherry-picked as 0400901 and pushed to 8.4.x to minimize conflicts going forward

Published change record.

Time to redo those stats for simpletest countdown - quite a few here - great work!

@jibran @dawehner probably have to update your drupalcon session slides too :-P

Haha :)

Thanks for the commit @larowlan! This sure does help with the countdown :)