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.
Simpletests to confirm that password errors and expire redirection work.
Note, because password policy prints the same text within the form as the form errors it becomes difficult to test for just the error conditions. Attached patch uses hook_form_alter() in the test module to remove that. There should be a better way to handle this, but providing here for early review.
Comments
Comment #2
AohRveTPV CreditAttribution: AohRveTPV commentedI think the patch may have failed testing because expiration occurs after the expiration time elapses, not when it elapses. The check for password expiration is this:
New patch tests this hypothesis by increasing the sleep to two seconds to exceed the expiration time of one second.
All tests pass for the original patch on my local system, but the discrepancy may be due to testbot having executed faster.
Comment #3
deekayen CreditAttribution: deekayen commentedComment #4
deekayen CreditAttribution: deekayen commentedComment #6
AohRveTPV CreditAttribution: AohRveTPV commentedtestPasswordSet is now consistently failing on testbot. The number of failures varies by run and is either 1 or 4. If I clone the code to my system, though, I can run the tests without failure.
Comment #8
AohRveTPV CreditAttribution: AohRveTPV commentedReverted commit and a subsequent related commit:
http://drupalcode.org/project/password_policy.git/commit/76872c9
http://drupalcode.org/project/password_policy.git/commit/e8b52c3
These failing tests were blocking other automated testing. Need to get testbot working with the tests. It is possible they are catching a bug in recently added code, but I cannot reproduce the failures independently. Need to at least understand why there is a difference in behavior.
Comment #10
AohRveTPV CreditAttribution: AohRveTPV commentedSame as #2, updated to apply to latest code. All tests pass on my system, but some should fail on testbot.
Comment #11
AohRveTPV CreditAttribution: AohRveTPV commentedAfter 30 minutes, test is queued and is not running. Maybe I'm being impatient, but I've never known it to take this long. Added a Testbots issue here: #2283213: Queued test not running.
Comment #12
AohRveTPV CreditAttribution: AohRveTPV commentedNow it passes on testbot. Will retest and commit if it passes again. Hopefully not some intermittent bug in Password Policy.
Comment #13
AohRveTPV CreditAttribution: AohRveTPV commented10: password_policy-ui-tests-2145649-10.patch queued for re-testing.
Comment #15
AohRveTPV CreditAttribution: AohRveTPV commentedNow it fails. Any ideas on how to debug this? Both runs were performed by the same test client.
Comment #16
AohRveTPV CreditAttribution: AohRveTPV commentedPatch adds an assertion and removes an ineffectual line of code. I'm wondering if the two consecutive password changes under
// Change password twice.
might be causing a race condition. I'd think drupalPost() would block, though.Comment #17
AohRveTPV CreditAttribution: AohRveTPV commentedComment #19
AohRveTPV CreditAttribution: AohRveTPV commentedThis patch removes nondeterminism from test inputs where the test is failing. Specifically, this eliminates any possibility randomName() would return the same value twice. Doubt it will help.
Comment #21
AohRveTPV CreditAttribution: AohRveTPV commented19: password_policy-ui-tests-2145649-20.patch queued for re-testing.
Comment #23
AohRveTPV CreditAttribution: AohRveTPV commentedWill submit an issue to Drupal.org Testbots and see if we can get verbose output, which would probably help debug this. That is my best idea on how to proceed. The behavior varies from run to run with testbot, but always passes on my local system. I've tried most suggestions from here: https://drupal.org/project/testbot
Comment #24
AohRveTPV CreditAttribution: AohRveTPV commentedPatch for which I hope we can get verbose output.
Comment #26
AohRveTPV CreditAttribution: AohRveTPV commentedOpened Testbots support request: #2283765: Test usually fails on testbot, but always passes locally
Comment #27
AohRveTPV CreditAttribution: AohRveTPV commented24: password_policy-ui-tests-2145649-24.patch queued for re-testing.
Comment #28
AohRveTPV CreditAttribution: AohRveTPV commented24: password_policy-ui-tests-2145649-24.patch queued for re-testing.
Comment #30
AohRveTPV CreditAttribution: AohRveTPV commentedUpdated to apply on latest code. Also switched to using an unserialized array to define the expire policy, since that is what is being done for other policies now. This patch will still usually fail due to the Testbots problems.
Comment #32
jthorson CreditAttribution: jthorson commented30: password_policy-ui-tests-2145649-30.patch queued for re-testing.
Comment #34
AohRveTPV CreditAttribution: AohRveTPV commentedTurns out I am able to reproduce the failures locally, only at a much lower rate. In 1 out of 50 recent runs, the failures occurred. So it does not appear to be a Testbots problem.
Comment #35
colanComment #38
AohRveTPV CreditAttribution: AohRveTPV commentedPatch adds some
sleep(1)
calls to check whether failures are a timing issue with the Past Passwords constraint.Comment #43
AohRveTPV CreditAttribution: AohRveTPV commented3 consecutive passes of #38 and 2 consecutive failures of #30 supports hypothesis of timing problem.
I think this code, which obtains the password history, is the problem:
The retrieved history is ordered in the array by 'created' time. If there are multiple history entries with the same time, the order will be unspecified. So, for instance, if three passwords are submitted in the same second what is supposedly the most recent password might actually be the third most recent password.
Testbot is probably executing the tests fast enough that the password changes in the test happen in the same second. (Not sure if there is a way to confirm this.)
The password history loading should be changed to order by some sequence number rather than time, so the order is guaranteed to be correct. Will fix the bug separately then again review #30.
Removing #38 as it was just for testing the timing hypothesis.
Comment #44
AohRveTPV CreditAttribution: AohRveTPV commentedComment #50
AohRveTPV CreditAttribution: AohRveTPV commentedUnfortunately my fix to ensure correct ordering of the password history did not solve the problem.
Still it seems to be a timing issue because adding the
sleep()
calls causes the tests to pass.Comment #54
AohRveTPV CreditAttribution: AohRveTPV commentedOK, I will try to reproduce these failures locally so I can more easily debug.
Comment #55
AohRveTPV CreditAttribution: AohRveTPV commentedAttempt to hack in some debug output, since I am having difficulty reproducing failures locally.
Comment #57
AohRveTPV CreditAttribution: AohRveTPV commentedPassword history entries were being added with
db_merge()
keyed on current time. So if two password changes happen in the same second, they would overwrite.I believe this will fix the failures.
Comment #58
AohRveTPV CreditAttribution: AohRveTPV commentedComment #61
AohRveTPV CreditAttribution: AohRveTPV commentedWent ahead and committed as I think it would be useful to have these tests now. They can be improved with further commits if necessary. Thanks, coltrane.