See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Convert

Everything except the views tests if #2863267: Convert web tests of views is not committed yet.
Everything not out of scope.

Out if scope for now

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

michielnugter created an issue. See original summary.

michielnugter’s picture

Status: Active » Postponed
michielnugter’s picture

michielnugter’s picture

Component: phpunit » node system
Issue summary: View changes
Status: Postponed » Active
Issue tags: +phpunit initiative
Lendude’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
7.19 KB

Tiny little start.

    $this->drupalGet('admin/reports/status');
    $this->clickLink(t('Rebuild permissions'));
    $this->drupalPostForm(NULL, [], t('Rebuild permissions'));
    $this->assertText(t('The content access permissions have been rebuilt.'));

This triggers the batch process in BrowserTestBase, so this fails. So changed to:
node_access_rebuild(FALSE);

Since the purpose of the rebuild is not to test the rebuild form but to just rebuild access, I find this acceptable but it does break the 'use the browser and not the API in browser tests'-rule. So very much up for discussion.

\Drupal\node\Tests\NodeEntityViewModeAlterTest needs #2795051: Move \Drupal\simpletest\WebTestBase::drupalBuildEntityView into a trait and make it available in BTB so moved out of scope till that lands

dawehner’s picture

+++ b/core/modules/node/tests/src/Functional/NodeAccessPagerTest.php
rename to core/modules/node/tests/src/Functional/NodeAccessRebuildNodeGrantsTest.php
index 251aa88268..53683f927b 100644

index 251aa88268..53683f927b 100644
--- a/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php

--- a/core/modules/node/src/Tests/NodeAccessRebuildNodeGrantsTest.php
+++ b/core/modules/node/tests/src/Functional/NodeAccessRebuildNodeGrantsTest.php

+++ b/core/modules/node/tests/src/Functional/NodeAccessRebuildNodeGrantsTest.php
@@ -69,10 +69,7 @@ public function testNodeAccessRebuildNodeGrants() {
 
     // Rebuild permissions.
-    $this->drupalGet('admin/reports/status');
-    $this->clickLink(t('Rebuild permissions'));
-    $this->drupalPostForm(NULL, [], t('Rebuild permissions'));
-    $this->assertText(t('The content access permissions have been rebuilt.'));
+    node_access_rebuild(FALSE);
 

@@ -105,10 +102,7 @@ public function testNodeAccessRebuildNoAccessModules() {
     // Rebuild permissions.
-    $this->drupalGet('admin/reports/status');
-    $this->clickLink(t('Rebuild permissions'));
-    $this->drupalPostForm(NULL, [], t('Rebuild permissions'));
-    $this->assertText(t('Content permissions have been rebuilt.'));
+    node_access_rebuild(FALSE);

Mhh, so I searched for 'Rebuild permissions' and the only instance of this in tests is exactly here. Wouldn't we loose test coverage if we no longer click this link?

Lendude’s picture

Issue summary: View changes

@dawehner hmm you are right (as per usual :), so that would require #2862885: Batch: Convert system functional tests to phpunit.

#2795051: Move \Drupal\simpletest\WebTestBase::drupalBuildEntityView into a trait and make it available in BTB is in, thanks for the quick review @dawehner and commit @alexpott! So NodeEntityViewModeAlterTest is back in scope.

dawehner’s picture

@Lendude
What about converting all beside the batch one?

Lendude’s picture

Assigned: Unassigned » Lendude
Issue summary: View changes
FileSize
21.91 KB
29.52 KB

@dawehner yeah moving it out of scope for now. Updated the IS.

This is still a work in progress. All Views tests still need to get done.

Moving \Drupal\node\Tests\NodeRevisionsTest out of scope, it has a bit where it tests the rendering of contextual filters, this should be split into a JavascriptTest.

In \Drupal\Tests\node\Functional\NodeTitleTest \Drupal\FunctionalTests\AssertLegacyTrait::assertTitle doesn't work with escaped strings, because the unescaped text is returned by the driver. So changed that to use assertRaw.
Same goes for \Drupal\Tests\node\Functional\NodeTitleXSSTest title test.

Converted SummaryLengthTest to a kernel test.
If multibyte support isn't enabled this throws an error: "Cannot use object of type Drupal\filter\Render\FilteredMarkup as array". Seems that mb_substr handles being passed a FilteredMarkup object fine, but the non-multibyte code flips out. Didn't dig into why, but seems unrelated.

Lendude’s picture

Bleh, both versions of NodeAccessRebuildNodeGrantsTest got stuck into that patch, ignore that, here is the real one.

The last submitted patch, 9: 2870453-9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2870453-10.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
14.68 KB
41.14 KB

Fixed the fails in #10. This covers everything unless its been put out of scope.

Bit dirty NodeRevisionPermissionsTest now uses TestBase::generatePermutations($parameters);. That method should be moved to a trait really, but this is a pattern that is used in some kernel tests currently, so if we move it to a trait we need to check for that anyway.

\Drupal\node\Tests\Views\NodeContextualLinksTest::renderContextualLinks is basically an empty test, but should be made into a javascript test that actually does some assertions.

RevisionRelationshipsTest converted to a kernel test, because it had no business being a functional test.

Lendude’s picture

NodeRevisionPermissionsTest now using the new GeneratePermutationsTrait. Interdiff is a bit wonky, still doesn't like --find-copies I think.

This is ready for a real review, everything that we can convert now is being converted.

dawehner’s picture

  1. +++ b/core/modules/node/tests/src/Functional/NodeTitleTest.php
    @@ -92,11 +92,11 @@ public function testNodeTitle() {
    -    $this->assertTitle($edge_case_title_escaped . ' | Drupal', 'Page title is equal to article\'s "title".', 'Node');
    +    $this->assertRaw($edge_case_title_escaped . ' | Drupal', 'Page title is equal to article\'s "title".', 'Node');
     
         // Test that the title appears as <title> when reloading the node page.
         $this->drupalGet('node/' . $node->id());
    -    $this->assertTitle($edge_case_title_escaped . ' | Drupal', 'Page title is equal to article\'s "title".', 'Node');
    +    $this->assertRaw($edge_case_title_escaped . ' | Drupal', 'Page title is equal to article\'s "title".', 'Node');
    
    +++ b/core/modules/node/tests/src/Functional/NodeTitleXSSTest.php
    @@ -32,7 +32,7 @@ public function testNodeTitleXSS() {
    -    $this->assertTitle(Html::escape($title) . ' | Drupal', 'Title is displayed when viewing a node.');
    +    $this->assertRaw(Html::escape($title) . ' | Drupal', 'Title is displayed when viewing a node.');
    

    Is there a reason we cannot use \Drupal\Tests\WebAssert::titleEquals

  2. +++ b/core/modules/node/tests/src/Kernel/SummaryLengthTest.php
    @@ -1,15 +1,82 @@
    -class SummaryLengthTest extends NodeTestBase {
    +class SummaryLengthTest extends KernelTestBase {
    

    Yeah, more kernel tests!

Lendude’s picture

#15.1 $this->getSession()->getPage()->find('css', 'title')->getText() returns the unescaped title, not the raw html, so either we check for the unescaped title, or we need to check raw. The original test tested against the escaped title and reading the comments around it that was the point of the test, so I went with assertRaw to keep the test more inline with the original.

But dove into the history of this test:
This test was added in #2560641: Remove all usages SafeMarkup::checkPlain() from render arrays and see @alexpotts comment here https://www.drupal.org/node/2560641#comment-10287861, getTitle() was modified in \Drupal\simpletest\AssertContentTrait::assertTitle to get this test to pass. Pretty clear comment there:
// Don't use xpath as it messes with HTML escaping.

So maybe we need to modify \Drupal\Tests\WebAssert::titleEquals too?

michielnugter’s picture

Status: Needs review » Needs work

Nice to see another big one getting this much progress!

So maybe we need to modify \Drupal\Tests\WebAssert::titleEquals too?

We could also open up an issue, do the change and see if all the tests still pass? I'm in doubt though, it would make titleEquals and exception in the way it works as compared to the rest of the assertion methods. I'm a fan of consistent API and breaking it should only be for a very good reason. Not sure if this is reason enough.

Some more review comments:

  1. +++ b/core/modules/node/tests/src/Functional/NodeRevisionPermissionsTest.php
    @@ -95,7 +100,7 @@ public function testNodeRevisionAccessAnyType() {
    -    $permutations = $this->generatePermutations($parameters);
    +    $permutations = TestBase::generatePermutations($parameters);
    

    This seems an incorrect change. The trait should make it available on $this.

  2. +++ b/core/modules/node/tests/src/Functional/NodeTitleTest.php
    @@ -92,11 +92,11 @@ public function testNodeTitle() {
    -    $this->assertTitle($edge_case_title_escaped . ' | Drupal', 'Page title is equal to article\'s "title".', 'Node');
    +    $this->assertRaw($edge_case_title_escaped . ' | Drupal', 'Page title is equal to article\'s "title".', 'Node');
    ...
    -    $this->assertTitle($edge_case_title_escaped . ' | Drupal', 'Page title is equal to article\'s "title".', 'Node');
    +    $this->assertRaw($edge_case_title_escaped . ' | Drupal', 'Page title is equal to article\'s "title".', 'Node');
    
    +++ b/core/modules/node/tests/src/Functional/NodeTitleXSSTest.php
    @@ -32,7 +32,7 @@ public function testNodeTitleXSS() {
    -    $this->assertTitle(Html::escape($title) . ' | Drupal', 'Title is displayed when viewing a node.');
    +    $this->assertRaw(Html::escape($title) . ' | Drupal', 'Title is displayed when viewing a node.');
    

    Not sure either on also updating assertTitle for BTB.
    If we don't update it this assertion seems to generic, it can be anyplace on the page. Should the check be more specific with the title tag in it?

  3. +++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
    @@ -82,7 +82,7 @@ public function testPublishedStatusNoFields() {
    -    $entity_id = $this->createEntity($values[$default_langcode], $default_langcode);
    +    $this->entityId = $this->createEntity($values[$default_langcode], $default_langcode);
         $storage = $this->container->get('entity_type.manager')
           ->getStorage($this->entityTypeId);
         $storage->resetCache([$this->entityId]);
    

    Hmm ok, how did this pass in Simpletest? Seems like the test was broken..

Lendude’s picture

Issue summary: View changes
FileSize
4.97 KB
41.2 KB

17.1 oops I missed one, fixed (and it had landed, ignore the stupid stuff I say during lunch ;)
17.2 yeah lets give it some more context, fixed
17.3 there are no mistakes, only happy accidents.....

Batch support has landed (yeah!) so moved \Drupal\node\Tests\NodeAccessRebuildNodeGrantsTest back into scope and it's in this patch and should be green

Lendude’s picture

Status: Needs work » Needs review

.....

michielnugter’s picture

Status: Needs review » Needs work

Did a thorough pass and it's almost there, I found 1 nit and had one question.

  1. +++ b/core/modules/node/tests/src/Functional/NodeTitleXSSTest.php
    @@ -32,7 +32,7 @@ public function testNodeTitleXSS() {
    -    $this->assertTitle(Html::escape($title) . ' | Drupal', 'Title is displayed when viewing a node.');
    +    $this->assertRaw(Html::escape($title) . ' | Drupal', 'Title is displayed when viewing a node.');
    

    Forgot one title assertion in adding the title tag.

  2. +++ b/core/modules/node/tests/src/Functional/NodeViewTest.php
    @@ -79,7 +79,7 @@ public function testLinkHeader() {
    -    $links = explode(',', $this->drupalGetHeader('Link'));
    +    $links = $this->drupalGetHeaders()['Link'];
    

    It was comparing to an array before but the conversion is missing the explode. Does the BTB version of drupalGetHeaders() return an array for the Link header already?

Lendude’s picture

Status: Needs work » Needs review
FileSize
4.2 KB
42.02 KB

@michielnugter thanks for the review as always.

#20.1 fixed
#20.2 see \Behat\Mink\Session::getResponseHeader. It only returns the first header with that name, not a concatenated string of all headers of that type like WebTestBase seems to do. drupalGetHeaders returns an array of arrays, so that seemed like the way to tackle this, no explode needed.

Moved \Drupal\Tests\content_translation\Functional\ContentTranslationOperationsTest into scope. It was already in the Drupal\Tests\content_translation\Functional namespace but still using the WebTestBase version of NodeTestBase, which can lead to weirdness.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I went through the patch again. The feedback from @michielnugter got adressed, so I think we are ready to go, right?

  • catch committed 4416f7b on 8.4.x
    Issue #2870453 by Lendude, michielnugter, dawehner: Convert web tests to...
catch’s picture

Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, and cherry-picked to 8.3.x, thanks!

  • catch committed bc65399 on 8.3.x
    Issue #2870453 by Lendude, michielnugter, dawehner: Convert web tests to...

  • catch committed 3971fcb on 8.3.x
    Revert "Issue #2870453 by Lendude, michielnugter, dawehner: Convert web...
catch’s picture

Version: 8.3.x-dev » 8.4.x-dev

Reverted from 8.3.x, we didn't commit #2863267: Convert web tests of views there.

catch’s picture

Version: 8.4.x-dev » 8.3.x-dev

Cherry-picked that issue, and have now cherry-picked this one.

  • catch committed 3d2ccc5 on 8.3.x
    Revert "Revert "Issue #2870453 by Lendude, michielnugter, dawehner:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.