Part of meta #1811016: [meta] Decouple tests from Node module.

PageCacheTest is yet another module that uses the node module just to check for a valid, rendered front page. I realized that making all these tests rely on user instead doesn't address the root problem. We need a reliable test page that always has the same test title and content, decoupled from any actual module.

See previous issues:

CommentFileSizeAuthor
#18 1811804-18-test-page-test-page-title.patch988 byteststoeckler
#15 1811804-15-test-page-test-page-title.patch923 byteststoeckler
#12 drupal-1811804-12.patch9.82 KBxjm
#12 interdiff.txt718 bytesxjm
#6 drupal-1811804-6.patch9.83 KBxjm
#6 interdiff-node-creation.txt951 bytesxjm
#2 interdiff.txt1.42 KBxjm
#1 system-1811804-1.patch9.58 KBxjm
test_page.patch8.16 KBxjm

Comments

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: Unassigned » xjm
StatusFileSize
new system-1811804-1.patch9.58 KB

Closed #1811622: Remove assumption about front page title from NodeCreationTest in favor of this issue; adding that. I'll look for others as well.

xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new interdiff.txt1.42 KB
xjm’s picture

xjm
she/her
Primary language English
commented

MenuTest may be another place to use this.

Status: Needs review » Needs work

The last submitted patch, system-1811804-1.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented 
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeCreationTest.phpundefined
@@ -100,10 +100,12 @@ function testFailedPageCreation() {
+    module_enable(array('test_page_test'));

This needs to go in $modules.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-node-creation.txt951 bytes
new drupal-1811804-6.patch9.83 KB

Fixed here. I checked and MenuTest probably wants to move away from a standard profile dependency, so leaving that out for now.

Status: Needs review » Needs work

The last submitted patch, drupal-1811804-6.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented

The fail above looked unrelated so I retested.

Related issue: #1811792: drupal_set_title() should not be necessary to set the homepage title

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs work » Needs review
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

This is much better than just arbitrarily checking 'user' or 'node' in different cases.

lars toomre’s picture

lars toomre commented
Status: Reviewed & tested by the community » Needs review

Overall this looks good @xjm. I will leave it as is for others to concur.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeCreationTest.phpundefined
@@ -100,10 +100,11 @@ function testFailedPageCreation() {
   function testUnpublishedNodeCreation() {
+    // Set the front page to the default "node" page.
+    config('system.site')->set('page.front', 'test-page')->save();

Small nit if this gets re-rolled. This in-line comment needs to be adjusted. This was a copy and paste from below.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff.txt718 bytes
new drupal-1811804-12.patch9.82 KB
lars toomre’s picture

lars toomre commented

Assuming this comes back green, given #10, RTBC!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

This looks good to me. Might be a tad bit more performant too, if we start making use of this test module widely wherever we actually mean "did a page come back ok?"

Committed and pushed to 8.x. I am going to go out on a limb and say testbot will not have anymore failures over a docs change than it did before (random fails notwithstanding).

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Fixed » Needs review
StatusFileSize
new 1811804-15-test-page-test-page-title.patch923 bytes

Awesome patch!
I had the same strange feeling when seeing the 'node' -> 'user' replacements.
One little thing:

+++ b/core/modules/system/tests/modules/test_page_test/test_page_test.module
@@ -0,0 +1,25 @@
+    'title' => 'Test front page',
...
+  drupal_set_title(t('Test page'));

I find the usage of drupal_set_title() very confusing here, and, unless I'm missing something, completely unnecessary.

Quick patch to remove that.
I think 'Test page' is more accurate than 'Test front page' as that depends on the usage.

xjm’s picture

xjm
she/her
Primary language English
commented

It is necessary; see #1811792: drupal_set_title() should not be necessary to set the homepage title.

vegabajalakes13’s picture

vegabajalakes13 commented

#6: drupal-1811804-6.patch queued for re-testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
StatusFileSize
new 1811804-18-test-page-test-page-title.patch988 bytes

Oh, I didn't know about that. Thanks! How about this one, then?:

vegabajalakes13’s picture

vegabajalakes13 commented

#6: drupal-1811804-6.patch queued for re-testing.

vegabajalakes13’s picture

vegabajalakes13 commented

1811804-6.patch-PageCacheTest module. Retest complete, test passed twice #17 and #19.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

I'm confused by that comment? It seems to be linking back to this issue?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Ok, right. So that was apparently just a copy/paste error cos the real link is over here #1811792: drupal_set_title() should not be necessary to set the homepage title.

xjm and I spoke about this, and since that issue needs tests anyway, rather than committing this as a temporary stop-gap, let's just make this change in that issue and assert that the title is set. Two birds, one patch. :D

Marking back to 'fixed' for the patch in #12.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: xjm » Unassigned

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.