The module tests fail. In the specific, it's testPasswordReset() to fail, with the consequence that all the automatic tests ran when a patch is submitted in a issue don't run, and keep waiting for the branch tests to pass.
I am setting the priority to Major, since this is a problem for many patches provided for this module.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | legal-3082676-5-branch-tests-fail.patch | 812 bytes | Rafal Lukawiecki |
Comments
Comment #2
avpadernoComment #3
Rafal LukawieckiThis is caused by the unnecessary, in my opinion, assert of the redirection path. That assert no longer works due to the outdated "?q=" fragment. I have bypassed that in the patch attached to https://www.drupal.org/project/legal/issues/2504177#comment-13737088. That patch passes with that assert removed.
You can see the value expected and the value returned in that assert in this test run: https://www.drupal.org/pift-ci-job/1750552 which shows:
If there is interest, I would be happy to create a patch just for the branch and this test—please let me know.
Comment #4
avpadernoThe only test I see checking that is
testPasswordReset(), which uses the following code.What seems wrong is the code for getting the current page path.
I am not sure it's necessary to verify the user sees the form to edit the account. The test already verifies the message shown to users using their one-time login link is present; that should be enough.
Comment #5
Rafal LukawieckiI agree. This section of the code should be removed:
I have already removed it in my patch for #2504177: Avoid redirecting to user page after legal acceptance and it passed the tests.
For what it is worth, the test fails because
$path="/"while$expected_path="/?q=user/3/edit"during the test run, which is caused by incorrect parsing of the URL (fragment vs path) which is likely to change dependent on how Drupal is installed.I attach a patch that removes this section, making the branch testable again.
Comment #6
Rafal LukawieckiComment #7
avpadernoTo make a comparison, this is the code used from
UserPasswordResetTestCase::testUserPasswordResetLoggedIn().It doesn't check on which page the users are redirect, but only if the You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password. message is shown on the page.
Definitively, the test of this module should just verify the message is present, without checking where the users are redirect.
Comment #8
avpadernoTests are passing. This is R&TBC.
@Rafal Lukawiecki Good work on finding the cause and fixing it!
Comment #9
Rafal LukawieckiThank you for your kind words and your help with this issue, @kiamlaluno.
Comment #11
robert castelo commentedThanks to everyone who helped get this issue fixed.