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.
Files: 
CommentFileSizeAuthor
#24 simpletest-1672764-24.patch1.76 KBiflista
PASSED: [[SimpleTest]]: [MySQL] 39,259 pass(es).
[ View ]
#22 simpletest-1672764-22.patch710 bytesiflista
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest-1672764-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 simpletest-1672764-20.patch13.45 KBiflista
PASSED: [[SimpleTest]]: [MySQL] 39,252 pass(es).
[ View ]
#18 simpletest-1672764-18.patch1.75 KBiflista
PASSED: [[SimpleTest]]: [MySQL] 39,265 pass(es).
[ View ]
#9 1672764-followup.patch816 bytesxjm
PASSED: [[SimpleTest]]: [MySQL] 37,262 pass(es).
[ View ]
#7 drupal-1672764-7.patch1.72 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 39,251 pass(es).
[ View ]
randomstring-docs.patch1.81 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,057 pass(es).
[ View ]

Comments

Issue summary:View changes

Updated issue summary.

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?

RTBC++

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

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

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

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

Status:Reviewed & tested by the community» Patch (to be ported)

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.72 KB
PASSED: [[SimpleTest]]: [MySQL] 39,251 pass(es).
[ View ]

See attached.

Status:Needs review» Reviewed & tested by the community

Version:7.x-dev» 8.x-dev
Status:Reviewed & tested by the community» Needs review
StatusFileSize
new816 bytes
PASSED: [[SimpleTest]]: [MySQL] 37,262 pass(es).
[ View ]

@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.

Status:Needs review» Reviewed & tested by the community

Seems like a good improvement.

Seems like good clarification to docs.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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

Moving back

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

Hm. I wonder who committed the fix? :)

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

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

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

Assigned:Unassigned» iflista
Status:Patch (to be ported)» Needs review
StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 39,265 pass(es).
[ View ]

Changed comments according to previous patches.
Needs review.

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.

Status:Needs work» Needs review
StatusFileSize
new13.45 KB
PASSED: [[SimpleTest]]: [MySQL] 39,252 pass(es).
[ View ]

Fixed few more comments longer than 80 character.

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!

Status:Needs work» Needs review
StatusFileSize
new710 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest-1672764-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Done.

Status:Needs review» Needs work

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

StatusFileSize
new1.76 KB
PASSED: [[SimpleTest]]: [MySQL] 39,259 pass(es).
[ View ]

Second try.

Status:Needs work» Needs review

Ups. Needs review! :)

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.

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

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.

Issue summary:View changes

Updated issue summary.