Problem/Motivation
I should be able to write a test that fails without crashing.
There are a number of SimpleTest assertions that crash rather than fail.
One of them is drupalLogin().
Typically, you do something like this:
$account = $this->drupalCreateUser(array('my special permission'));
$this->drupalLogin($account);
I should be able to write that test, run it, and have it fail without a code error, even before I implement hook_permission().
drupalCreateUser() does the right thing by returning FALSE if my permission doesn't exist.
drupalLogin() does the WRONG thing by using type hinting instead of checking its input. drupalLogin() should give me a fail if I don't have a valid user, rather than a code error.
Proposed resolution
Remove type hinting from drupalLogin(), and have it show a fail if the account object is invalid. Also, short-circuit the remaining login process.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#19 | drupallogin_crashes-2234295-19.patch | 1.8 KB | Anonymous (not verified) |
Comments
Comment #1
Mile23This patch removes the type hinting and does an assertion testing whether $account is an AccountInterface or not.
Also it bails on the process if it's not an AccountInterface.
Comment #2
Mile23Comment #3
klausiChanging drupalLogin() seems like the wrong approach. I would rather suggest that drupalCreateUser() always returns an account object, and if the permission does not exist it should probably invoke $this->fail() and just create the user without the permission. We are already doing that as far as I can see in checkPermissions(), so we can just continue creating the role/account without returning false.
Comment #4
Mile23There's simply no way in which an invalid user logged in helps my tests become more meaningful.
There's also no way in which an invalid user logged in helps me save time on testing.
So let's test whether the user is valid, and be done.
Comment #5
sunIgnoring the error does not help in any way — the test will not be able to proceed and only result in failures, which in turn are almost guaranteed to blow up in a later call chain.
Instead of ignoring the error,
drupalCreateRole()
should properly halt the test (method) execution by throwing an exception.Comment #7
Mile23Uhm, no. :-)
The goal here is to write tests that do not throw exceptions or code errors when they fail.
I should be able to write a test that fails without a code error. That way I can convert D7 tests to D8 before I write any other code. I should be reasonably secure in the idea that if there's a code error, I haven't finished converting the test yet. This is not a safe assumption to make with the current SimpleTest.
Basically all of these methods should act in isolation, failing if they can't do what the test says to do, but not throwing exceptions or code errors. It's not an ERROR that the role doesn't exist, or that the user doesn't exist, it's a FAIL of the TEST.
That is: They SHOULD ignore the error, because the error is not in the test, the error is in the system under test. And the error in the system under test is exactly what we're testing for. :-)
Fails are good. :-)
Comment #8
sunThe only way to do that is to not execute functional code within the test class; i.e., you can use the internal browser to manually perform a POST request to the user registration form to create a new test user account.
As long as your test class/method executes functional code that operates on actual services, entities, and data objects, that code is not able to execute, if basic constraints are not met (like any other functional code outside of tests) — the code will throw exceptions and fatal errors anyway.
Web tests are programmatically creating user roles and user accounts through functional code, because doing these operations via HTTP requests would be very slow (which would significantly slow down the full core test suite). In almost all cases, these operations are peripheral to the actual test (whereas tests explicitly asserting the user role/account functionality are not using the helper methods), so that is why it is legit to perform them programmatically.
Comment #9
Mile23Yes, exactly. I'm not sure how this is supposed to convince me that it's totally OK for SimpleTest to throw errors while failing to generate a fixture. I'm also kind of amazed that having SimpleTest not throw errors is at all controversial.
Also, I will address those subsequent errors in other issues. :-) See for example: #1452896: PHP notice in clickLink if link does not exist.
Comment #10
sunSimpletest itself does not throw errors. Your custom integration code (= test class) makes it throw errors. Why do you expect broken code to work?
Comment #11
Mile23That is my point. It does and shouldn't.
Nope.
Here, let me show you the test that started this. Because I'm a modern developer who believes in TDD and stuff, I'm converting D7 tests to D8 for the Examples project. I wrote a test for menu_example, with a skeleton module and only the test. Here's the result:
https://qa.drupal.org/pifr/test/764843
You'll have to navigate down into the MenuExampleTest section.
This is a test that does nothing wrong. It is not in error. There is no PHP error in it. It is not calling any SimpleTest methods incorrectly or doing anything weird at all. There is no error in Drupal, and there is no error in my test. The assertions should simply fail, because they haven't been implemented yet.
But instead of just seeing a bunch of fails, I see a bunch of errors. This makes it needlessly difficult to work on the test, or on the module, because SimpleTest is shouting at me that it stubbed its toe, when all I care about is whether the test passed or failed. It's difficult to know whether my code is bad or whether SimpleTest is WTF.
Here's the issue for that test result: https://drupal.org/node/1883724#comment-8654343
Comment #12
tim.plunkettYes, and that's why #1452896: PHP notice in clickLink if link does not exist is an actual issue (and has been for 2 years).
I'd like to think you're talking past each other, but I'm not sure.
Comment #13
Mile23We seem to be arguing about whether drupalLogin() should throw a PHP error when it receives FALSE instead of a user object.
I'm not sure why.
Anyone care to review the patch? :-)
Comment #14
tim.plunkettI think the patch in #5 is great, and would help resolve a major frustration I've had writing tests.
The patch in #1 would mask the problem, and prevent finding the issue. It could also cause issues if a test *does* pass regardless of authentication failing.
Comment #15
Mile23The current behavior is to report non-existent problems. #1 changes that to causing failing tests to report failure, without inventing problems that don't exist.
If the user isn't logged in, and your test of the behavior of authenticated users passes when there's no authenticated user, then SimpleTest isn't the problem. Besides, drupalLogin() will still report a failure, even if it doesn't scream BS error messages at you.
Clearly my objection doesn't register, so I'll just quit. SimpleTest wants to continue to suck.
Comment #16
Mile23Re-roll of #1.
Restating the problem:
When SimpleTest fails to create a role with a permission, it is not an error, but a failed test of that permission's existence. Failing to create a user with that role is likewise not an error but a failed test of the permission's existence. Subsequent failures due to having no logged in user with a given role are consequences of the failed test, not errors.
This patch to drupalLoginUser() asserts that the user it's asked to log in is a user object. It then gives a debug message about being unable to log in if it's not an account object.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedNo reroll needed, but fixed the test failures on the info message from simpletest.
Comment #20
Mile23+1 on #19. :-)
Comment #21
daffie CreditAttribution: daffie commentedLooks good to me.
Comment #22
alexpottIsn't the real issue here that we continue to run a test after the first failure?
Still needs tests see #2
Comment #31
quietone CreditAttribution: quietone as a volunteer commentedTriaging issues in simpletest.module as part of the Bug Smash Initiative to determine if they should be in the Simpletest Project or core.
This looks like it a Phpunit issue, changing component.
Comment #32
alexpottWe no longer continue on fail cause PHPUnit is sensible. So the approach in #5 looks nice.
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis came up as a daily bugsmash target. Looking at #5 since simpletest has been removed is this still an issue?
When simpletest was removed https://www.drupal.org/node/3091784