Updated: Comment #26
Problem/Motivation
Follow-up of #2004408: Allow modules to alter the result of EntityListController::getOperations and #2020209: Random failures due to non uniqueness of randomString() and randomName() and #1988780: DrupalWebTestCase::buildXPathQuery() tries to handle backreferences in argument values
This issue originally was solving the issue fixed in #1988780: DrupalWebTestCase::buildXPathQuery() tries to handle backreferences in argument values however in testing we have discovered another set of circumstances where using random strings will cause assertLink to fail - if it contains 2 consecutive spaces see #2076929: EntityOperationsTest random fail for an example.
This issue is caused by the fact that we normalise spaces in assertLink()
$links = $this->xpath('//a[normalize-space(text())=:label]', array(':label' => $label));
Proposed resolution
Add the ability to validate random strings during generation and if invalid generate another.
Remaining tasks
Get reviews
User interface changes
N/a
API changes
- $validator param added to
\Drupal\Component\Utility\Random::string()
it's optional. - Methods on
\Drupal\Component\Utility\Random
are no longer static which makes unit testing possible
Original report
See revisions - this has been extensively rewritten.
Comment | File | Size | Author |
---|---|---|---|
#36 | random-fail-ouch-2032453-36.patch | 826 bytes | Berdir |
#30 | 26-30-interdiff.txt | 2.95 KB | alexpott |
#30 | 2032453.randomstring-validator.30.patch | 14.46 KB | alexpott |
#26 | 23-26-interdiff.txt | 981 bytes | alexpott |
#26 | 2032453.randomstring-validator.26.patch | 14.43 KB | alexpott |
Comments
Comment #1
alexpottIt also will fail if it contains two consecutive spaces...
Comment #3
alexpottHere's a patch that adds the ability to validate a randomString
What is interested is that these tests proved we have issues with tests bleeding to each because they where using Drupal::config functions.
No interdiff because this approach is quite different... it uses PHPUnit to mock the abstract...
Comment #4
alexpottAdded test case from #2032565: Test failures due to special character (combinations) in random strings and improved test strings... you can never have too many pastes.
Comment #4.0
alexpottUpdated issue summary.
Comment #5
tsphethean CreditAttribution: tsphethean commentedThinking about a slightly lower impact change, can we not just replace preg_replace with str_replace in the WebTestBase::buildXPathQuery method? It seems like we're only doing relatively simple string replacement so may get away with not using the regex.
It passes my sample test in the original post, curious what it does to all the other tests.
Comment #6
alexpott#5 yes but... this does not cope with all the issues that #4 does... we also have issues with consecutive spaces and also starting and ending with a space as the xpath normalises spaces.
And I'm pretty sure there are other combinations we have not come up with yet which will fail for different random reasons. That's why I think an implementation based on #4 is the way to go. Since it will be easy just to add the additional validations to the callback.
Comment #7
alexpottJust to prove #6 copied values from #4 into the test from #5
Comment #8
alexpottI've cleaned up an unnecessary abstraction that was in #4...
Comment #9
alexpottThinking about this some more... I think the change from
preg_replace<code> to <code>str_replace
that #5 makes is the right thing to do and will mean that the validator callback can be simpler as we won't have to avoid strings that look like regex back references.Comment #10
tsphethean CreditAttribution: tsphethean commentedI think I agree - I get the same test failures locally on the double whitespace and leading/trailing whitespace issues with the patch from #7, but the tests that I've been looking at (i.e. using drupalCreateRole) do a trim() of randomString anyway with comments:
We could patch randomString further to not generate spaces in the string, so
chr(mt_rand(32, 126))
becomes
chr(mt_rand(33, 126))
Comment #11
tsphethean CreditAttribution: tsphethean commentedThis patch adds a trim on the rolename which createRole() does on the randomly generated string, eliminates chr(32) from randomString generation and removes the double whitespace tests as they would no longer be possible.
Comment #12
alexpottWe should not remove the space character from random strings... also trimming the rolename will fix other places where we can be caught out by this.
I did not mean that #5 was the entire fix... we need to combine #5 and #8...
Comment #13
alexpottSo here is the combination of #5 and #8
We protected against:
Regex back references are no longer a since switching to
str_replace()
- nice catch @tsphetheanI've added test coverage for assertLink - unfortunately PHPUnit testing this method proved overly complicated since it is protected - see testing your privates :)
Comment #15
alexpottOkay so making the random class static prevents it's reuse and makes it impossible to unit test as each test changes the starting conditions :(
Patch attached fixes that...
Comment #17
alexpottDoh!
Comment #18
BerdirComment #19
BerdirYou asked for it ;)
shouldn't this reference the string method?
\Drupal\...
typo (Asert)
While adding tests anyway, should we also add a test to make sure it doesn't match a link that's not there, and maybe a string that's not a link?
The tricky part about the preg_replace() is the word boundary (\b), we are removing this here.
I've no idea if it is necessary, but I assume there was a reason why we put it here. I'm quite sure that this was written by @DamZ, so we should probably ask him.
In CacheCollectorTest, I documented this as "\PHPUnit_Framework_MockObject_MockObject", this gives you autocomplete on mocking methods. There was some discussion about it, but it got in and I think it's quite useful.
missing () ?
That's what we put in after all these reviews? ;)
...
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedNack. It is not ok to remove the boundary check, as it makes
:placeholder1
match for:placeholder11
. Of course, the boundary check is not foolproof either, but it is better then nothing.The easy fix for this issue described here is
preg_replace_callback()
with a lambda.Comment #21
alexpottThanks for the reviews!
Comment #22
alexpottPressed save too soon... The will fail patch in #21 proves that using preg_replace_callback() will fix our issue whilst preserving the boundary check. Thanks @damz
Comment #23
alexpottSo I've just discovered and committed #1988780: DrupalWebTestCase::buildXPathQuery() tries to handle backreferences in argument values so this issue will just handle the addition of the random string validation callback so that strings generated by it don't contain 2 spaces in a row or start or end with a space.
Comment #24
catch#2076929: EntityOperationsTest random fail was dupicate.
Comment #25
BerdirI thought we had fixed this one, haven't seen this fail since the referenced issue above was committed.
Comment #26
alexpottThis patch now prevents a much much rarer error - when there are two consecutive spaces inside a random string! Which is exactly what's occurred in #2076929: EntityOperationsTest random fail
This patch makes the random not a public static as this make it extremely difficult class to unit test and there does not seem to be any reason for it being so.
Reviewed and noticed a something wrong in the comments.
Comment #26.0
alexpottMerging issue summary from https://drupal.org/node/2032565
Comment #27
alexpottTagging
Comment #27.0
alexpottUpdated
Comment #28
BerdirIn case someone is wondering, type hint on callable is only possible with PHP 5.4+
Testing Simpletest with PHPUnit. Interesting... ;)
I think we don't shorten variable names, so this should be $string.
Can you approve your own API change? Or do you need someone else to do that? I can't, but other than the above nitpick, can't find anything wrong with it, have reviewed the same code a few times already :)
Comment #29
catchI think he can but so can I on API changes.
Comment #30
alexpottAddressed point 3 from #28
Comment #31
BerdirOk, let's do this. I have nothing to complain about anymore.
Comment #32
catchCommitted/pushed to 8.x.
Needs a change notice.
Comment #33
catch#2083415: [META] Write up all outstanding change notices before release
Comment #34
tstoecklerNot setting back to needs work, but I'm not sure that changing Random::$strings and Random::$names to no longer be static was a worthwhile API change. I agree on making the methods non-static but I don't see any reason why the variables were changed. Before you could rely on the fact that Random would generate a string (if wanted) that had not been generated during the entire request. This is no longer the case. Now you have to make sure that you're dealing with the exact same object. I'm not aware of any concrete implications but I also don't see this being discussed anywhere or the original issue with Random referenced, which contains a lot more discussion about this topic.
Comment #35
andypostI can confirm that I got a few false failures in the test
RandomTest::testRandomStringValidator()
for example https://qa.drupal.org/pifr/test/625203
Comment #36
BerdirConfirmed, this does fail randomly, always with the same test assertion:
See attached patch. With that, not getting any failures with --repeat 10000.
Comment #37
alexpottMakes sense - nice catch
Comment #38
catchCommitted/pushed to 8.x, thanks!
Comment #40
xjmThe change notice still hasn't been written.
Comment #40.0
xjmUpdated issue summary.
Comment #41
mr.baileysMy first attempt at writing a change record:
Change record
Description
\Drupal\Component\Utility\Random::string()
. The parameter takes a callable that returns TRUE if the generated string is valid, FALSE otherwise. In case of an invalid string, Random::string() will attempt to generate a new random string until the string is valid orMAXIMUM_TRIES
is reached.Before
After
The $validator callable can be used to discard randomly generated strings that are invalid. The example below ensures that randomly generated strings do not start with a digit.
Comment #42
mr.baileysSetting to needs review for the change record.
Comment #43
alexpott#41 looks great. Once you've created a https://drupal.org/node/add/changenotice - then you can just mark this issue as fixed. Thanks!
Comment #44
mr.baileysChange notice added: https://drupal.org/node/2175701