Only use standard profile where necessary in comment.module tests (10% bot speed-up) [#1812034]

Problem/Motivation

The comment module tests are slow, redundant, hard to follow, and make all sorts of assumptions.

Proposed resolution

Convert CommentTestBase to not use the standard profile.
Have certain tests enable the additional modules they require.
Add the standard profile to several other tests that were not easily decoupled from standard (some rely on markup etc.; see #1272870: No semantics for nested comments / bad for screen-readers).
Move several test methods (and their respective helpers) out of the rather bloated CommentInterfaceTest into new files.

Make the following change to the commentExists() helper method:

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
@@ -17,20 +20,60 @@
@@ -122,7 +165,6 @@ function commentExists(Comment $comment = NULL, $reply = FALSE) {

@@ -122,7 +165,6 @@ function commentExists(Comment $comment = NULL, $reply = FALSE) {
     if ($comment) {
       $regex = '/' . ($reply ? '<div class="indented">(.*?)' : '');
       $regex .= '<a id="comment-' . $comment->id . '"(.*?)'; // Comment anchor.
-      $regex .= '<div(.*?)'; // Begin in comment div.
       $regex .= $comment->subject . '(.*?)'; // Match subject.
       $regex .= $comment->comment . '(.*?)'; // Match comment.
       $regex .= '/s';

This div is present in Bartik but not in Stark.

Address additional cleanups in followup issues.

Remaining tasks

Related issues

Comment	File	Size	Author
#25	comment-1812034-25.patch	58.55 KB	xjm
#25
#24	comment-1812034-24b.patch	51.84 KB	xjm
#24
#22	comment-1812034-22.patch	51.7 KB	xjm
#22
#21	comment-1812034-21.patch	60.87 KB	xjm
#21
#19	comment-1812034-19.patch	51.43 KB	xjm
#19
#13	comment-1812034-13.patch	24.66 KB	xjm
#13
#10	comment-1812034-10.patch	14.51 KB	xjm
#10
#7	1812034-7.patch	4.69 KB	xjm
#7
#6	1812034-5.patch	4.24 KB	xjm
#6
	comment-tests.patch	2.53 KB	xjm

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

xjm

she/her

English

CreditAttribution: xjm commented 13 October 2012 at 20:03

Status:

Active

» Needs review

Comment #2

xjm

she/her

English

CreditAttribution: xjm commented 13 October 2012 at 20:11

I'm seriously considering rewriting them from scratch.

Comment #3

13 October 2012 at 20:36

Status:

Needs review

» Needs work

The last submitted patch, comment-tests.patch, failed testing.

Comment #6

xjm

she/her

English

CreditAttribution: xjm commented 13 October 2012 at 21:28

Assigned:	Unassigned	» xjm
Status:	Needs work	» Needs review
Issue tags:		+Testing system

File	Size
1812034-5.patch	4.24 KB

Comment #7

xjm

she/her

English

CreditAttribution: xjm commented 13 October 2012 at 21:41

File	Size
1812034-7.patch	4.69 KB

Uhm so yeah. commentExists() depends on... Bartik markup. Good times!

Comment #8

13 October 2012 at 22:09

Status:

Needs review

» Needs work

The last submitted patch, 1812034-7.patch, failed testing.

Comment #10

xjm

she/her

English

CreditAttribution: xjm commented 13 October 2012 at 22:52

Status:

Needs work

» Needs review

File	Size
comment-1812034-10.patch	14.51 KB

Comment #11

13 October 2012 at 23:23

Status:

Needs review

» Needs work

The last submitted patch, comment-1812034-10.patch, failed testing.

Comment #12

xjm

she/her

English

CreditAttribution: xjm commented 14 October 2012 at 00:55

Title:

Comment module tests need cleanup

» Only use standard profile where necessary in comment.module tests

Narrowing scope, because I'm seeing a significant performance increase locally already.

Comment #13

xjm

she/her

English

CreditAttribution: xjm commented 14 October 2012 at 01:59

Status:

Needs work

» Needs review

File	Size
comment-1812034-13.patch	24.66 KB

Comment #14

14 October 2012 at 02:31

Status:

Needs review

» Needs work

The last submitted patch, comment-1812034-13.patch, failed testing.

Comment #15

Lars Toomre CreditAttribution: Lars Toomre commented 14 October 2012 at 10:16

Looking at the patch in #13 @xjm, it was not immediately apparent to me where the failures were coming from.

One thing I did notice, though, is that you have hit upon several of the tests that violate our coding standards. Class members like $this->admin_user should be $this->adminUser (e.g, use camelCase naming convention). Similarly, $user->web_user should be $this->webUser.

I opened up #1811638: [meta] Clean-up Test members - ensure property definition and use of camelCase naming convention to address a similar problem I found in the Contact module tests. Perhaps you want to add a comment there if the variable renaming does not occur in this issue?

Comment #16

xjm

she/her

English

CreditAttribution: xjm commented 14 October 2012 at 17:13

Re: #15, this patch is a work in progress. I do know where the test failures are coming from, and I am deliberately uploading partial patches to expose the failures as I work on changing away from the standard profile.

Comment #17

xjm

she/her

English

CreditAttribution: xjm commented 14 October 2012 at 17:34

Oh, another note -- renaming classes or member variables is outside the scope of this issue, because having changed lines in a file makes git unable to detect the renames, and changing the names of $admin_user and $web_user will result in hundreds of changed lines throughout these tests (which are huge), making the patch enormously difficult to review.

I agree with updating the documentation and standards in lines that are already changed so long as they don't affect other lines, and I will probably improve the docblocks for the changed tests. This patch will probably take a minute or two off every D8 testbot run on d.o, and so is worth committing as soon as it is functional, rather than making additional cleanups here.

Comment #19

xjm

she/her

English

CreditAttribution: xjm commented 14 October 2012 at 21:40

Status:

Needs work

» Needs review

File	Size
comment-1812034-19.patch	51.43 KB

Trying again. Better approach.

Comment #20

14 October 2012 at 22:13

Status:

Needs review

» Needs work

The last submitted patch, comment-1812034-19.patch, failed testing.

Comment #21

xjm

she/her

English

CreditAttribution: xjm commented 14 October 2012 at 22:28

Status:

Needs work

» Needs review

File	Size
comment-1812034-21.patch	60.87 KB

Comment #22

xjm

she/her

English

CreditAttribution: xjm commented 14 October 2012 at 22:33

File	Size
comment-1812034-22.patch	51.7 KB

Whoops, #21 totally contains another patch. Fixed here.

Comment #23

14 October 2012 at 23:08

Status:

Needs review

» Needs work

The last submitted patch, comment-1812034-22.patch, failed testing.

Comment #24

xjm

she/her

English

CreditAttribution: xjm commented 15 October 2012 at 00:05

Status:

Needs work

» Needs review

File	Size
comment-1812034-24b.patch	51.84 KB

Well, fine then.

Comment #25

xjm

she/her

English

CreditAttribution: xjm commented 15 October 2012 at 01:09

File	Size
comment-1812034-25.patch	58.55 KB

Not duplicating a whole test method this time.

Comment #26

xjm

she/her

English

CreditAttribution: xjm commented 15 October 2012 at 02:57

Okay cool. So here's what this patch does:

Converts CommentTestBase to not use the standard profile.
Has certain tests enable the additional modules they require.
Adds the standard profile to several other tests that were not easily decoupled from standard (some rely on markup etc.; see #1272870: No semantics for nested comments / bad for screen-readers).
Moves several test methods (and their respective helpers) out of the rather bloated CommentInterfaceTest into new files.

Makes the following change to the commentExists() helper method:

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentTestBase.phpundefined
@@ -17,20 +20,60 @@
@@ -122,7 +165,6 @@ function commentExists(Comment $comment = NULL, $reply = FALSE) {

@@ -122,7 +165,6 @@ function commentExists(Comment $comment = NULL, $reply = FALSE) {
     if ($comment) {
       $regex = '/' . ($reply ? '<div class="indented">(.*?)' : '');
       $regex .= '<a id="comment-' . $comment->id . '"(.*?)'; // Comment anchor.
-      $regex .= '<div(.*?)'; // Begin in comment div.
       $regex .= $comment->subject . '(.*?)'; // Match subject.
       $regex .= $comment->comment . '(.*?)'; // Match comment.
       $regex .= '/s';

This div is present in Bartik but not in ~~standard~~ Stark.

I think this is a good start, and it reduces test run times a lot for me locally. It would probably be best to commit this first and then do additional cleanups later, since this patch mostly just moves code around.

Not in scope: any documentation, code style, or DX cleanups for the moved code.

Comment #27

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 15 October 2012 at 14:59

Title:	Only use standard profile where necessary in comment.module tests	» Only use standard profile where necessary in comment.module tests (10% bot speed-up)
Status:	Needs review	» Reviewed & tested by the community

If I read the logs on qa.drupal.org correctly, this brings down the total test time of Drupal core from 30 min to 27 min (-10%) !!! Awesomesauce!!! (adding this to the title because it's so awesome)

Looked through this and everything looks good. Let's do this.

Comment #27.0

xjm

she/her

English

CreditAttribution: xjm commented 15 October 2012 at 15:13

Issue summary:

View changes

Updated issue summary.

Comment #27.1

xjm

she/her

English

CreditAttribution: xjm commented 15 October 2012 at 15:14

Issue summary:

View changes

Updated issue summary.

Comment #28

xjm

she/her

English

CreditAttribution: xjm commented 15 October 2012 at 15:15

Thanks @tstoeckler!

I updated the issue summary based on the final patch.

Comment #28.0

xjm

she/her

English

CreditAttribution: xjm commented 15 October 2012 at 15:15

Issue summary:

View changes

Updated issue summary.

Comment #29

xjm

she/her

English

CreditAttribution: xjm commented 15 October 2012 at 16:11

FWIW I also put the QA test results for this patch side-by-side with the 8.x branch tests, stared, counted, and made sure no assertions had gone missing (adding up the counts from CommentInterfaceTest, CommentStatisticsTest, CommentLinksTest, CommentNewIndicatorTest, and CommentCSSTest to get a total of 2005, the expected one assertion more than the 2004 CommentInterfaceTest has in the branch tests). Also, everything looks pink to me now.