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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23’s picture

FileSize
1.07 KB

This 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.

Mile23’s picture

Status: Active » Needs review
Issue tags: +Needs tests
klausi’s picture

Status: Needs review » Needs work

Changing 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.

Mile23’s picture

There'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.

sun’s picture

Status: Needs work » Needs review
FileSize
1.8 KB

Ignoring 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.

Status: Needs review » Needs work

The last submitted patch, 5: drupal8.test-user-role-exception.5.patch, failed testing.

Mile23’s picture

Uhm, 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. :-)

sun’s picture

The goal here is to write tests that do not throw exceptions or code errors when they fail.

The 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.

Mile23’s picture

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.

Yes, 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.

sun’s picture

Simpletest itself does not throw errors. Your custom integration code (= test class) makes it throw errors. Why do you expect broken code to work?

Mile23’s picture

Simpletest itself does not throw errors.

That is my point. It does and shouldn't.

Your custom integration code (= test class) makes it throw errors.

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

tim.plunkett’s picture

Yes, 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.

Mile23’s picture

We 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? :-)

tim.plunkett’s picture

I 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.

Mile23’s picture

The patch in #1 would mask the problem, and prevent finding the issue.

The 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.

It could also cause issues if a test *does* pass regardless of authentication failing.

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.

Mile23’s picture

Status: Needs work » Needs review
FileSize
1012 bytes
1012 bytes

Re-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.

Status: Needs review » Needs work

The last submitted patch, 16: 2234295_16.patch, failed testing.

The last submitted patch, 16: 2234295_16.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
1.8 KB

No reroll needed, but fixed the test failures on the info message from simpletest.

Mile23’s picture

+1 on #19. :-)

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Isn't the real issue here that we continue to run a test after the first failure?

Still needs tests see #2

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

Component: simpletest.module » phpunit

Triaging 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.

alexpott’s picture

We no longer continue on fail cause PHPUnit is sensible. So the approach in #5 looks nice.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +Bug Smash Initiative

This 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

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.