From #345428: drupalLogin uses wrong function to create new user it was pointed out that if no user object is passed to DWTC>drupalLogin(), a new user is automatically created. Currently, that codes does not work since it incorrectly calls DWTC->_drupalCreateRole() instead of DWTC->drupalCreateUser().

For the sake of consistency in tests, my solution is to remove the default user creation from DWTC->drupalLogin(). I did a search for = ?\$this->drupalLogin|\$this->drupalLogin\((NULL)?\) across all .test files in core and contrib, and only found two core .test results. Both of them never actually use the return value (the created user) from DWTC->drupalLogin(). We don't even have a test to make sure that it works, since it's been using the wrong function this entire time. If this 'feature' is not used by any core or contrib tests, why do we need it? Let's leave it up to the tests to make sure they create the appropriate and correct users.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Status: Active » Needs review
FileSize
3.48 KB

Patch provided for review.

Damien Tournoud’s picture

Status: Needs review » Closed (duplicate)

Dave, I agree that drupalLogin() should have only one purpose, and should not attempt to create a user. But, this is really a duplicate of #345428: drupalLogin uses wrong function to create new user. I know that the original poster is not really listening, but this issue should be discussed in one place only.

Dave Reid’s picture

Status: Closed (duplicate) » Needs review

With the current code:

$test_user = $this->drupalLogin();

What kind of permissions does $test_user have? Do they have no permissions? This is cause for tests to not perform as desired.

Whereas with:

$default_user = $this->drupalCreateUser();
$this->drupalLogin($default_user);

$test_user = $this->drupalCreateUser(array('access content'))
$this->drupalLogin($test_user);

$no_permissions_user = $this->drupalCreateUser(array());
$this->drupalLogin($no_permissions_user);

I specify the exact permissions I need for the test and am guaranteed I know what that user can do.

Both codes will have the same number of assertions performed, but one is easier to understand what is going on.

Dave Reid’s picture

Status: Needs review » Closed (duplicate)

Thanks Damien...fixing cross-post.

Darren Oh’s picture

Status: Closed (duplicate) » Needs review
Damien Tournoud’s picture

Status: Needs review » Closed (duplicate)

Per agreement from the original poster.

Pancho’s picture

Status: Closed (duplicate) » Reviewed & tested by the community

Per agreement with catch, Dave and Damien: RTBC

keith.smith’s picture

Status: Reviewed & tested by the community » Needs work

The patch in #1 has a setup the a clean environment in a comment.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
3.48 KB

Well, shucks thanks for finding that. That section was just some trailing whitespace that is auto-corrected by my IDE, so I didn't even read over the comment. Thanks!

keith.smith’s picture

Status: Needs review » Reviewed & tested by the community

Back to rtbc after comment was fixed.

Pancho’s picture

crossposted: No need to remove rtbc status. The miswording was not introduced by this patch, it just wasn't fixed by it.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.