Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We rely on chance to ensure that drupalCreateRole and drupalCreateUser do not fail if called multiple times in the same test. This is a bad idea.
Comment | File | Size | Author |
---|---|---|---|
#41 | 38-41-interdiff.txt | 1.64 KB | alexpott |
#41 | 2020209.randomuniques.41.patch | 13.69 KB | alexpott |
#38 | 37-38-interdiff.txt | 2.61 KB | alexpott |
#38 | 2020209.randomuniques.38.patch | 13.59 KB | alexpott |
#37 | 36-37-interdiff.txt | 766 bytes | alexpott |
Comments
Comment #1
alexpottAnd here's a patch...
Comment #2
alexpottBlah my .htaccess file crept in :(
Comment #3
Berdir:)
Same description for both?
Missing type
Comment #4
alexpottThanks!
Comment #5
YesCT CreditAttribution: YesCT commentedshould this be
isset($strings... ?
copy and paste typo?
Comment #6
alexpottThanks @YesCT and @chx pointed out no need for these functions to be static too...
Comment #7
YesCT CreditAttribution: YesCT commentedsome more copy and paste fix.
also,
https://drupal.org/node/1354#classes
Comment #9
alexpott#7: 2020209.randomuniques.7.patch queued for re-testing.
Comment #10
alexpottYet another c&p error from me :( spotted by @YesCT
Comment #11
Gábor HojtsyLooks great. I think it would have been equally fine to have this feature in the existing methods (as a base feature), but additional methods sound equally fine. This highlights the specifics of the behaviour definitely.
Comment #12
alexpottI agree with #11...
Comment #13
alexpott... as a result now is the time to make randomName and randomString non static...
Comment #14
Gábor HojtsyThe *Unique methods don't exist anymore.
Comment #15
alexpottDoh!
Comment #16
Gábor HojtsyLooks good, thanks for putting it into the original implementation I think it will not hurt to have this unique at all times. I don't think people expect randomname to return sometimes the same values as before randomly :)
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedThis looks great to me as well. RTBC+1. @alexpott: since that's now two people who've reviewed it, I think it's okay for you to commit it, despite it being your patch.
Comment #18
alexpottCommitted 880d568 and pushed to 8.x. Thanks!
Comment #19
tstoecklerCan someone explain how the $this->randomStrings and $this->randomNames are being set? It's not obvious to me from the patch.
Comment #20
Gábor HojtsyWups, looks like it does not remember the old strings in fact.... They will always be an empty array....
Comment #21
Gábor HojtsyComment #22
Gábor HojtsyLooks better now? :)
Comment #23
Berdir\Drupal\Tests\UnitTestCase has it's own implementation of randomName() that we should fix too.
Comment #24
alexpottSo I've refactored randomName, randomString and randomObject into a new random component and added tests for the uniqueness of random names and strings. Additionally I have added a fail safe to ensure we don't end up in an endless loop.
I have ran the new phpUnit test over 1000 times locally to ensure this test does not introduce new random fails.
Comment #25
xjmLooks great other than some stupid docs nitpicks.
Defines a.... https://drupal.org/node/1354#classes
Let's keep these lines in
TestBase
.generation.
s/less/fewer/ :)
Comment #26
alexpottThanks @xjm
Comment #27
BerdirThis doesn't need to be inside the loop, it's static.
Comment #28
alexpottGood point...
Comment #29
alexpottOk missed the assertion messages :)
Comment #31
Berdir#29: 2020209.randomuniques.29.patch queued for re-testing.
Comment #32
BerdirWe don't actually document anywhere that it *does* loop and does ensure unique random strings per request.
I think we should a) do that and b) provide a possibility to disable the uniqueness check, especially because there's no way to reset it and we explicitly want to make this available to non-test code.
Maybe that should even be the default and only the test methods that call out to this should enforce uniqueness?
$max also doesn't change :)
Comment #33
alexpottThanks for the review @berdir...
Comment #34
alexpottSo lets prove we have the problem...
Comment #35
dawehneryou will never know that maybe something changes stuff here and it potentially break. Let's ensure the size of the object in a test.
Comment #36
alexpott@dawehner here is a test for you...
I test 0 and 1 because different amounts of code will fire in Random::object depending on this value. So we have complete execution path coverage.
Based on the result of #34 marking this as major as random testbot failures greatly impact our velocity.
Comment #37
alexpottComment #38
alexpottImproving docs...
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentednitpick alert:
need visibility
Comment #40
Berdirnitpick alert^2.
Should start with (optional), maybe something like "If uniqueness of returned strings should be enforced, defaults to TRUE" ?
Comment #41
alexpottThanks for the reviews :)
Comment #42
ParisLiakos CreditAttribution: ParisLiakos commentedthanks!
Comment #43
catchThat looks better. Committed/pushed to 8.x, thanks!
Comment #44
BerdirMore random fun, here is the random problem that @alexpott and I discovered in Dublin: #2032565: Test failures due to special character (combinations) in random strings.