testDrupalPreRenderLinks() uses DOMDocument().
Anything using assertThemeOutput uses assertIdentical() on check_plain(var_export($output, TRUE))

And now #1038932-43: theme_image_formatter() assumes that title, alt, and options are always set is creating a menu callback and using xpath.

Ideally you would know the EXACT expected output, but in D7 any theme function that generates a link will need a workaround for clean URLs.

CommentFileSizeAuthor
#13 drupal-1706878-13.patch2.04 KBtim.plunkett
#8 drupal-1706878-8.patch1.03 KBlazysoundsystem
#6 drupal-1706878-6.patch1.02 KBlazysoundsystem
#2 drupal-1706878-2.patch3.1 KBtim.plunkett
#1 drupal-1706878-1.patch3.09 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Standardize on a method of testing theme function output » Allow all modules to use the method for testing theme function output
Priority: Major » Normal
Status: Active » Needs review
StatusFileSize
new drupal-1706878-1.patch3.09 KB

This would certainly be a start.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new drupal-1706878-2.patch3.1 KB

Change the t() to format_string() to match our code standards.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Allow all modules to use the method for testing theme function output » Add WebTestBase::assertThemeOutput() to allow modules to test theme function output
Status: Needs review » Reviewed & tested by the community

Thanks! Clarifying title.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Needs backport to D7

Might as well backport this.

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Looks good to me. Committed/pushed to 8.x.

Some other tests may have copied assertThemeOutput() already but I think it's probably safe to backport, up to David and Angie anyway.

lazysoundsystem’s picture

lazysoundsystem commented
Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new drupal-1706878-6.patch1.02 KB

While updating #983528: support $group and $message parameters in all assert() functions for D8, I noticed that this test doesn't return anything.

I've added that in a patch against the already committed version.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work 
+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -2320,6 +2320,8 @@ abstract class WebTestBase extends TestBase {
    *   (optional) An assertion message.
+   * @return
+   *   TRUE on pass, FALSE on fail.

There should be a blank line before @return

lazysoundsystem’s picture

lazysoundsystem commented
Status: Needs work » Needs review
StatusFileSize
new drupal-1706878-8.patch1.03 KB

Thanks @andypost, good point. I'll get the hang of this one day...

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Good to go

lars toomre’s picture

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

In the future, please add type hinting to all @param and @return directives for any docblock you update. Thanks.

lars toomre’s picture

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

Whoops... did not mean to change status.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 8.x-dev » 7.x-dev
Category: task » feature
Status: Reviewed & tested by the community » Patch (to be ported)

Doesn't look like anything else in that file has type hinting, so probably best to handle that in a separate patch as part of the larger clean-up effort. It would be weird to introduce that here and nowhere else in the file/function.

Committed and pushed follow-up to 8.x. Back to 7.x. Should be harmless to backport. Re-classifying as a feature request though.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new drupal-1706878-13.patch2.04 KB
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Straight backport.

David_Rothstein’s picture

David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Not sure if it's a feature request or a task, but either way it looks good :)

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/eb1be63

David_Rothstein’s picture

David_Rothstein commented

No idea why the "7.18 release notes" tag fails to show up on the issue half the time I add it in a comment... trying again.

David_Rothstein’s picture

David_Rothstein commented

Drupal 7.18 was a security release only, so this issue is now scheduled for Drupal 7.19 instead.

Fixing tags accordingly.

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein commented

Drupal 7.19 was a security release only, so this issue is now scheduled for Drupal 7.20 instead.

Fixing tags accordingly.

David_Rothstein’s picture

David_Rothstein commented

Drupal 7.20 was a security release only, so this issue is now scheduled for Drupal 7.21 instead. For real this time... I think :)

Fixing tags accordingly.

David_Rothstein’s picture

David_Rothstein commented

Trying to fix the tags again...

David_Rothstein’s picture

David_Rothstein commented
Issue tags: +7.22 release notes

Fixing tags since Drupal 7.21 only contained a fix to deal with fallout from the Drupal 7.20 security release. Hopefully this is really the last time I do this and this will be released in Drupal 7.22 for real :)