One-time login link test does not delete its user

This seems to do the job.

Thanks @trevjs!

Some review:

1. +++ b/tests/features/login.feature
   @@ -18,7 +18,7 @@ I want to be able to login
   -    Given I am on "/user/login"
   +   Given I am on "/user/login"
   

   Random indentation change.

2. +++ b/tests/steps/panopoly_test.behat.inc
   @@ -366,25 +366,15 @@ class TestSubContext extends BehatContext implements DrupalSubContextInterface {
   -   * @Given /^I log in with the One Time Login Url$/
   +   * @Given /^I log in with the One Time Login Url as "([^"]*)"$/
       */
   -  public function iLogInWithTheOneTimeLoginUrl() {
   +  public function iLogInWithTheOneTimeLoginUrl($username) {
        if ($this->getMainContext()->loggedIn()) {
          $this->getMainContext()->logOut();
        }
    
   -    $random = new Random;
   -
   -    // Create user (and project)
   -    $user = (object) array(
   -      'name' => $random->name(8),
   -      'pass' => $random->name(16),
   -      'role' => 'authenticated user',
   -    );
   -    $user->mail = "{$user->name}@example.com";
   -
   -    // Create a new user.
   -    $this->getMainContext()->getDriver()->userCreate($user);
   +    // load the user
   +    $user = user_load_by_name($username);
   

   This is the meat of the patch! It's taking a (hopefully) existing username, rather than creating a new one.

   However, I don't think this is quite right. We don't really know that that user exists, and we're not removing it anywhere.

I think creating a new user with a randomly generated username is fine. We just need to make sure to delete it after the tests are finished.

That step is adding the new users to $this->users, however, now that this in a sub-context, rather than part of the main context (like it used to be before #2293747: Move our FeatureContext and *.features into panopoly_test so that child distributions can reuse them), we're no longer removing those users later.

I think what we need is just a method tagged @AfterScenario that loops through those users and deletes them. For example (untested):

/**
 * @AfterScenario
 */
public function removeUsers() {
  foreach ($this->users as $account) {
    user_delete($account->uid);
  }
}

Thanks for the review. I'll put in a patch for the indent at least, however, as far as I can tell this does remove the user. Let me outline my thinking, and you can correct me if I'm wrong.

At the start of the feature we have this to set up a user named TestUser:

Background:
    Given users:
    | name     | pass      | mail             | roles    |
    | TestUser | ChangeMe! | foo@example.com  | 

And in the two previous scenarios we are passing in the name of this user. Example

  @standard_login @api
  Scenario: Editor is able to login
    Given I am on "/user"
    When I fill in "TestUser" for "edit-name"
    And I fill in "ChangeMe!" for "edit-pass"
    And I press "Log in"
    Then I should see "Log out"

The Background/Given users does appear to clean up for itself, and so my approach was to change the one time login test to also pass in the name of "TestUser" and rewrote the step to accomodate this. Prior to this it was creating a new user in the step with a random username and that is how we were ending up with users that weren't being cleaned up. I changed it to use "TestUser" because this approach seemed consistent with the previous two tests.

Does that make sense? Or was there a reason we wanted to use a randomly generated user rather than the "TestUser" created at the start of the feature? Perhaps we need some sort of check to see if the user exists?

Let me know what you think and I'll put in another patch.

Ah, ok, so it's depending on the "Given users" step. However, I'd still prefer just to fix the way we were doing it before, ie. removing the new users we're creating at the end of the Scenario.

Something along these lines? This checks at the end of every scenario.

See below...

This seems to work. I was having some trouble using xdebug, but the random user is removed.

Something along these lines? This checks at the end of every scenario.

Yeah, that's pretty much what I was thinking, thanks! Can you make two changes:

1. Declare the variable towards the beginning of the class, for example:
   

   /**
    * Holds a list of temporary users created by our steps.
    *
    * @var array
    */
   protected $users = array();
   

2. At the end of your removeUsers() method, clear out $this->users - otherwise it'll continue to accumulate values with each scenario run.

Thanks! One last bit of review:

+++ b/tests/steps/panopoly_test.behat.inc
@@ -306,6 +314,21 @@ class TestSubContext extends BehatContext implements DrupalSubContextInterface {
+      unset($this->users);

Rather than unset() this, let's set it to an empty array like:

$this->users = array();

This way that member variable is always a valid array (which is part of the reason I wanted to declare it as well).

I was thinking the same thing.

Looks great, thanks! Committed.