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.The drupalLogin() function currently contains two assertions that are guaranteed to "pass" no matter what:
- The 'blocked message' test makes sure a particular phrase does not appear on the page, but this phrase will never appear since Drupal outputs a different message in that case (and has for a long time, I believe)...
- The 'reserved message' test is even better; not only is it trying to ensure the absence of a phrase that can never be there, but the feature itself was removed from Drupal core over a year ago :) See #228594: UMN Usability: split access rules into an optional module
This patch simply removes both tests, since there really is no reason to run three separate assertions here when one will do. Also, this is a good example of why tests that use assertNoText() and similar functions are inherently fragile and should be avoided whenever possible -- I can only imagine how many other bugs like this exist :)
However, in removing them, we also need to improve the one test that does remain (since searching for the username on the page is not a good indicator that the user is logged in; the name might appear in an error message as is the case for blocked users). I went with something simple that is far from perfect, but at least is much better than what is currently there.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | assert_link_followup.patch | 1.19 KB | David_Rothstein |
| #9 | remove_drupalLogin_assertions_followup_2.patch | 1.29 KB | David_Rothstein |
| #6 | remove_drupalLogin_assertions_followup.patch | 880 bytes | David_Rothstein |
| remove_drupalLogin_assertions.patch | 1.29 KB | David_Rothstein | |
Comments
Comment #1
brianV CreditAttribution: brianV commentedRemoving the two redundant assertions is a good idea.
However, determining if a user is logged in by the presence of a 'logout' link is a little weak, because that is only guaranteed to work on a very vanilla site - one small theme change could rename or remove the logout link, or replace it with an image etc. etc.
Perhaps a better test would be to try to access /user/logout - if the user is not logged in, that page will return an access denied. Furthermore, I can't think of any reason this behaviour would be changed on any website.
Comment #2
brianV CreditAttribution: brianV commentedOk, recieved some flak in IRC over the above comment. After well-reasoned discussion with DamZ & Berdir, I think I'll withdraw the second part of my post above.
Patch looks good - RTBC.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein commentedIf it's any consolation, my first reaction when I read your comment was "that sounds like a much better way to do it!" -- I had to think for a couple minutes before I realized why it wouldn't work :)
Actually, I think the first part of your comment might not be a concern either? If I understand SimpleTest correctly, it always uses the default install profile when running tests, so it will always run them under Garland regardless of what theme you have enabled on your site. If that's true, then I think the only way this could be a problem is for people who have hacked Garland, which we don't need to support.
So, RTBC as long as the testbot approves it.
Comment #4
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #5
Dave ReidNice improvement!
Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedCall me prophetic, but I wrote "RTBC as long as the testbot approves it" and apparently I meant it :)...
I did NOT test the patch before submitting it, and I think it was committed before the testbot ran. Trying it locally, I am getting a bunch of test failures, due to a bit of a boneheaded mistake (we check the return value of assertLink but this function doesn't return anything).
Here is the followup patch which I think should fix things. Note that if it isn't committed ASAP, HEAD may be broken.
Comment #7
brianV CreditAttribution: brianV commentedMy fault. I should have not set it to RTBC without the testbot hitting it. In fact, I hadn't thought I had... but the record shows I did ;)
Apologies all around.
Comment #8
Dave ReidWe need the same return for assertNoLink then as well.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedOK, here is a version that does that - these appear to be the only assertion functions that don't return anything, so I think we're covered for the future.
I'm told that the testbot is currently offline right now anyway, so that is probably a good thing. I'll run the tests locally and make sure they all pass with this patch.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein commentedOK, the tests essentially all pass for me locally, although after that it returns with "The test run did not successfully finish" anyway -- not sure what's up with that, but it happens with or without any of the code posted in this issue, so it's something else entirely...
This is RTBC and should go in quickly to fix the tests.
Comment #11
Dries CreditAttribution: Dries commentedCommitted. Thanks!
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedNow that the chaos is averted, here is one more minor followup - we should make the PHP Docblock for the return value of these functions match that of all the other ones.
Sorry for all the followups on this issue :)
Comment #13
webchickCommitted!
Hopefully fixed for good this time. :) Thanks for the attention to detail!