Problem/Motivation

  • Misuse of the method DrupalTestBase::randomString() is frequently responsible for random, hard-to-debug test failures, because it generates a list of random characters that includes special characters like #, *, /, \, ?, etc.
  • Developers often do not consider the implications of using this for values that are not user input or that have already been sanitized, nor do they necessarily know to look for DrupalTestBase::randomName() (which is the "safe" variant).

Proposed resolution

  • Improve the documentation to clarify when each method should be used.

Remaining tasks

  • Patch needs review.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue summary: View changes

Updated issue summary.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This looks reasonable to me, and the documentation is clear.... Maybe I'll wait for a second opinion (or a few days at RTBC) to commit though, in case the policy suggestion here needs tweaking?

tstoeckler’s picture

RTBC++

jhodgdon’s picture

Thanks! I also just left messages for the two simpletest maintainers in IRC about this issue to see if they want to comment.

chx’s picture

Looks good. Do we want namespace anchors already? Or fix 'em later?

Dries’s picture

Version: 8.x-dev » 7.x-dev

Committed to 8.x. Moving to 7.x.

xjm’s picture

Status: Reviewed & tested by the community » Patch (to be ported)
tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.72 KB

See attached.

chx’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs review
FileSize
816 bytes

@sun's remarks in #1659682: Consider renaming randomString() and/or adding additional, context-appropriate helpers for random string generation made me realize my wording might inappropriately dissuade developers from using randomString() when they should. The attached is better, I think. Apologies for the extra work.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Seems like a good improvement.

boombatower’s picture

Seems like good clarification to docs.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

tim.plunkett’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

Moving back

jhodgdon’s picture

Looks good, committed to 8.x, ready to port original + follow-up to 7.x.

Note: the 7.x patch should probably wait on this "avoid commit conflicts" patch for committing:
#1541958: Split setUp() into specific sub-methods

jhodgdon’s picture

Hm. I wonder who committed the fix? :)

tim.plunkett’s picture

@jhodgdon you did, guess Dries didn't push?

http://drupalcode.org/project/drupal.git/commit/586d48e

xjm’s picture

Note that #1541958: Split setUp() into specific sub-methods is against 8.x again now.

iflista’s picture

Assigned: Unassigned » iflista
Status: Patch (to be ported) » Needs review
FileSize
1.75 KB

Changed comments according to previous patches.
Needs review.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! I think the text in the patch looks good, but this line needs to be wrapped to 80 character maximum:

+   * machine or file names that have already been validated); instead, use DrupalWebTestCase::randomName().

Should be a quick re-roll... I still think we need to wait for #1541958: Split setUp() into specific sub-methods before this is committed also.

iflista’s picture

Status: Needs work » Needs review
FileSize
13.45 KB

Fixed few more comments longer than 80 character.

jhodgdon’s picture

Status: Needs review » Needs work

Please limit the patch on this issue to fixing just this one issue in 7.x that was already fixed in 8.x.

Other documentation cleanups are being addressed on:
#1310084: [meta] API documentation cleanup sprint
If you'd like to help with that other issue, that would be great! Instructions are at the top of that issue. Thanks!

iflista’s picture

Status: Needs work » Needs review
FileSize
710 bytes

Done.

Status: Needs review » Needs work

The last submitted patch, simpletest-1672764-22.patch, failed testing.

iflista’s picture

FileSize
1.76 KB

Second try.

iflista’s picture

Status: Needs work » Needs review

Ups. Needs review! :)

jhodgdon’s picture

Title: Improve documetation of randomString() and randomName() » Improve documentation of randomString() and randomName()
Status: Needs review » Reviewed & tested by the community

Looks good! I'll get this committed shortly.

jhodgdon’s picture

Oh, I can't commit this until
#1541958: Split setUp() into specific sub-methods
is done.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

That patch is now in, and it looks like it didn't wind up conflicting (but thanks for waiting anyway).

So, I went ahead and committed this one to 7.x. Thanks! http://drupalcode.org/project/drupal.git/commit/1c2301b

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.