@D7csmtl

Related issue: http://drupal.org/node/601308

Because garland is the default theme, and it over-rides lots of templates, tests in node.test were not breaking on broken html code in the node.tpl.php found in node.module. To get around this, I propose we use the Stark theme as the default theme in simpletest. Thanks to Omar who put me on to this idea.

Marked as critical, because I feel it is, but feel free to downgrade.

CommentFileSizeAuthor
#23 use-stark-for-testing-719814-22.patch1.07 KBsfyn
#17 add-tests-to-default-templates-719814-17.patch597 bytesrjgoldsborough
#15 719814-drupal-badcamp-testing-profile-use-stark-theme-15.patch597 bytesmlncn
#5 simpletest-719814-5.patch583 bytessfyn
#1 simpletest-719814-1.patch545 bytessfyn

Comments

sfyn’s picture

sfyn commented
StatusFileSize
new simpletest-719814-1.patch545 bytes

@D7csmtl

Here is my patch to use stark as the simpletest default theme.

sfyn’s picture

sfyn commented
Status: Active » Needs work
sfyn’s picture

sfyn commented
Status: Needs work » Needs review

Go testbot go

Status: Needs review » Needs work

The last submitted patch, simpletest-719814-1.patch, failed testing.

sfyn’s picture

sfyn commented
Status: Needs work » Needs review
StatusFileSize
new simpletest-719814-5.patch583 bytes

reroll

Status: Needs review » Needs work

The last submitted patch, simpletest-719814-5.patch, failed testing.

sfyn’s picture

sfyn commented
Issue tags: -D7csmtl

Looking at the details of this patch I am impressed at how many fails it generated. I'm calling it a night because I'm not sure where to start right now.

UPDATE
I had a look at some of the failing tests:

403 functionality (AccessDeniedTestCase) [System]
The two failed passes assert the text 'User login' on custom and default 403 pages. My manual tests with Stark worked.
Admin theme block admin accessibility (BlockAdminThemeTestCase) [Block]
The message "The block admin page for a disabled theme can not be accessed" is generated by an assertResponse for a 403 that goes to Stark's block page. It fails if stark is enabled.
Menu link creation/deletion (MenuTestCase) [Menu]
Creates and tests several menu links, modifying them and verifying their presence on various pages. 28 fails. The fails are in verifyMenuLink and toggleMenuLink . . .
OpenID login and account registration (OpenIDFunctionalTest) [OpenID]
47 Fails in functions that test openID auto registration.
Forum functionality (ForumTestCase) [Forum]
16 Fails. Most of them appear to me to be related to testing breadcrumb display in the main forums page and specific forume

Overall, 14 test cases had fails. Most of these on a few lines of code, repeatedly. I will have a try at getting more passes out of this patch over the next few days.

sfyn’s picture

sfyn commented
Issue tags: +D7csmtl

Tagged as part of the Drupal 7 Montreal codesprint

boombatower’s picture

boombatower commented
Version: 7.x-dev » 8.x-dev
Priority: Critical » Normal
Issue tags: +D7csmtl

Err...what!? We are far beyond making such a significant change for D7.

sfyn’s picture

sfyn commented

I understand that this is a significant change, but I am worried about broken templates sneaking into the final release. I would like to try reworking the 14 failing test cases . . .

But if its going on to Drupal 8, how about allowing tests to run under a theme chosen when you launch testing / or by default testing under each core theme?

sfyn’s picture

sfyn commented
Priority: Normal » Major

Without knowing the current state of this in 8, I am bumping up to major in the hopes it gets some attention.

lars toomre’s picture

lars toomre commented

This bug came up for me today on bug bingo; hence, my response here.

The Drupal SimpleTest methodology seems exhaustively to run all tests before a new patch is applied. I would hope/expect that all theme-related tests are run against all themes that are included as part of a Drupal distribution. Similarly, I would expect that all "plugable" code elements are tested before a patch is said to 'pass'. Hence, I would think that this patch is just a subset of selecting one theme array element from a list of several core themes.

While it has been some time since the proposed patch was last rolled, I am bothered that such a simple change would break so many tests. I would suggest that the next roll of this patch check each and every one of the themes included Drupal 7/8.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Now that we have the testing profile, we should use that.

Basically just add the code from the patch above to testing_install().

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Simpletest does not test system default templates (like node.tpl.php) » Add tests for default templates (like node.tpl.php)
Category: bug » task
Priority: Major » Normal

This is a task, not a bug.

mlncn’s picture

mlncn commented
Status: Needs work » Needs review
StatusFileSize
new 719814-drupal-badcamp-testing-profile-use-stark-theme-15.patch597 bytes

Arrived at tstoeckler's conclusions independently (here at BADCamp with Scor, Webchick, Jacine), and here is the patch.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Novice

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

rjgoldsborough’s picture

rjgoldsborough commented
Status: Needs work » Needs review
StatusFileSize
new add-tests-to-default-templates-719814-17.patch597 bytes

Rerolled for d8.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Novice

Thanks!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Needs work

I hate to put this to "needs work" again, but there should be a theme_disable(array('bartik')); as well in there.
Also, in case #1181776: Change theme_default variable to Stark makes it, this is obsolete.

rjgoldsborough’s picture

rjgoldsborough commented
Status: Needs work » Closed (duplicate)

Looks like #1181776: Change theme_default variable to Stark is fixed so marking this as a duplicate. Please feel free to fix if this is the incorrect action to take.

sfyn’s picture

sfyn commented
Status: Closed (duplicate) » Needs work

I am reopening, because I do not understand how making stark the default theme for the minimal install changes this situation - the install profile used for testing is the testing profile, as I understand it.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Closed (duplicate)

@sfyn: Yes, #1181776: Change theme_default variable to Stark only changed the default theme in core (and the Testing profile) to Stark, but very few tests are using the Testing profile thus far. That last step is a larger challenge and is why the major test rewrite patch in #1373142: Use the Testing profile. Speed up testbot by 50% exists.

sfyn’s picture

sfyn commented
Status: Closed (duplicate) » Needs review
StatusFileSize
new use-stark-for-testing-719814-22.patch1.07 KB

Here is the new patch with the theme disable call from #19

sfyn’s picture

sfyn commented
Status: Needs review » Closed (duplicate)

Apologies. I misunderstood the stark issue when I scanned it earlier.