Add login/logout Nightwatch commands (and a Drupal "login" command to allow for that)

Reposting patch from #2972229: Expand the JS test coverage using nightwatch:

👋I'm new to this but made an attempt to add a login, logout, and isLoggedIn command to our Nightwatch instance. Test coverage for the new commands are included.

Edit: We could also have a PHP script that returns an appropriate session cookie based on a user ID, then call that with child_process, if setting a username and password feels weird to people.

Edit 2: Another piece of self-review - the isLoggedIn command accepts a callback because the getCookies method is asynchronous, but the login and logout commands do not accept a callback even though they call isLoggedIn. Should they also take a callback? I researched this a little bit and it looks like with asynchronous _stuff_ in Nightwatch, you have to use callbacks, but can end up nesting many levels deep. Thoughts?

23:54:53 PHP Warning:  file_put_contents(/var/lib/drupalci/workspace/jenkins-drupal_patches-58861/source/core/.env): failed to open stream: Permission denied in /opt/drupalci/testrunner/src/DrupalCI/Plugin/BuildTask/BuildStep/Testing/NightwatchJS.php on line 62
23:54:53 sudo BABEL_DISABLE_CACHE=1 -u www-data yarn --cwd /var/www/html/core test:nightwatch
23:55:00 yarn run v1.6.0
23:55:00 $ cross-env BABEL_ENV=development node -r dotenv-safe/config -r babel-register ./node_modules/.bin/nightwatch --config ./tests/Drupal/Nightwatch/nightwatch.conf.js
23:55:00 
23:55:00 /var/www/html/core/node_modules/dotenv-safe/index.js:31
23:55:00             throw new MissingEnvVarsError(allowEmptyValues, options.path || '.env', example, missing, dotenvResult.error);
23:55:00             ^
23:55:00 MissingEnvVarsError: The following variables were defined in .env.example but are not present in the environment:
23:55:00   DRUPAL_TEST_BASE_URL, DRUPAL_TEST_DB_URL, DRUPAL_TEST_WEBDRIVER_HOSTNAME, DRUPAL_TEST_WEBDRIVER_PORT, DRUPAL_TEST_CHROMEDRIVER_AUTOSTART, DRUPAL_NIGHTWATCH_OUTPUT, DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES
23:55:00 Make sure to add them to .env or directly to the environment.
23:55:00 
23:55:00 If you expect any of these variables to be empty, you can use the allowEmptyValues option:
23:55:00 require('dotenv-safe').config({
23:55:00   allowEmptyValues: true
23:55:00 });
23:55:00 
23:55:00 Also, the following error was thrown when trying to read variables from  .env:
23:55:00 ENOENT: no such file or directory, open '/var/www/html/core/.env'
23:55:00     at Object.config (/var/www/html/core/node_modules/dotenv-safe/index.js:31:19)
23:55:00     at /var/www/html/core/node_modules/dotenv-safe/config.js:9:18
23:55:00     at Object.<anonymous> (/var/www/html/core/node_modules/dotenv-safe/config.js:10:3)
23:55:00     at Module._compile (module.js:652:30)
23:55:00     at Object.Module._extensions..js (module.js:663:10)
23:55:00     at Module.load (module.js:565:32)
23:55:00     at tryModuleLoad (module.js:505:12)
23:55:00     at Function.Module._load (module.js:497:3)
23:55:00     at Module.require (module.js:596:17)
23:55:00     at Function.Module._preloadModules (module.js:753:12)
23:55:00 error Command failed with exit code 1.

hmmm

This failed due to a permissions issue on the test bot,

23:54:53 PHP Warning:  file_put_contents(/var/lib/drupalci/workspace/jenkins-drupal_patches-58861/source/core/.env): failed to open stream: Permission denied in /opt/drupalci/testrunner/src/DrupalCI/Plugin/BuildTask/BuildStep/Testing/NightwatchJS.php on line 62

Addressed #6. Additional assertions should not be needed, although the test does looks funny without them. :-)

Putting back into needs review, but would like someone else to look at my "Edit 2" comment from #2. I also now wonder if "getCookies" is actually asyncronous, or if it just happens to take a callback.

@dawehner I guess I was just trying to avoid runtime errors, but we could remove the check so users could at least see an error (ex: Uncaught TypeError callback.call is not a function).

According to @lauriii this is a blocker for the JS Nightwatch testing stuff.

User login is useless as it is if we don't allow passing the roles or permissions while creating the user.

@lauriii just pointed me to #2974619: Create nightwatch command to install modules I think creating a console command to create the user with given role/permissions is the way to fix #11.

Yesterday @Wim Leers mentioned on a call that we could use #2403307: RPC endpoints for user authentication: log in, check login status, log out for logging in, instead of using the login form.

I still think that generic console commands i.e. creating a user and installing module would be really helpful but these goals do make sense to me.

For any interaction which needs to create stuff like enabling modules, creating content types etc.
we use a browser, like moving to /admin/modules check the checkbox for a module and hit save

We'd need a user 1 for that, right?

We will create new commands for all this setup functionality

You mean nightwatch commands not symfony console commands?

Optimizing for test performance is something we can worry later, if needed.

We can always run JS and PHP tests in parallel.

Login the user by default
[…]
This way we decouple ourselves from the API

+1, this seems like a great initial direction.

Created a new patch based on #15.

➡ #19.1 - this gets a bit confusing, i gave it a shot but couldn't get it working.
➡ #19.2 - fixed
➡ #19.3 - fixed
➡ #19.4 - fixed
➡ #19.5 - php help please.

1. Fixed #19.5.

2. +++ b/core/tests/Drupal/Nightwatch/Commands/drupalCreateRole.js
   @@ -0,0 +1,57 @@
   +        .expect.element('.user-role-form .machine-name-value').to.be.visible.before(2000);
   ...
   +      this.perform((done) => {
   

   Added a comment to explain it.

3. Fixed some CS issues.

+++ b/core/tests/Drupal/Nightwatch/Commands/drupalCreateRole.js
@@ -22,6 +22,8 @@ exports.command = function drupalCreateRole({ permissions, name = null }, callba
+        /* Wait for the machine name to appear so that it can be used later to
+         select the permissions from the permission page. */

This comment format should be like,

/**

*/

Thanks, for the review. I follow the core/tests/Drupal/Nightwatch/Commands/drupalUninstall.js comment pattern.

Fixes #24.

> I'm wondering whether it would be sensible to have some level of test coverage here as well.

Going to write a PHP test for the console command.

1. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteUserLoginCommand.php
   @@ -0,0 +1,88 @@
   +  /**
   +   * The database prefix of this test run.
   +   *
   +   * @var string
   +   */
   +  protected $databasePrefix;
   

   AFAICT We don't need this. This seems to be copy/pasted from TestSiteInstallCommand, which is a special case.

2. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteUserLoginCommand.php
   @@ -0,0 +1,88 @@
   +  /**
   +   * Time limit in seconds for the test.
   +   *
   +   * Used by \Drupal\Core\Test\FunctionalTestSetupTrait::prepareEnvironment().
   +   *
   +   * @var int
   +   */
   +  protected $timeLimit = 500;
   

   Same.

3. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteUserLoginCommand.php
   @@ -0,0 +1,88 @@
   +      ->addArgument('uid', InputArgument::OPTIONAL, 'The id of the user to which the link will be generated', 1)
   

   s/id/ID/
   s/to/for/
   s/which/whom/

   Also, why isn't this required?

4. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteUserLoginCommand.php
   @@ -0,0 +1,88 @@
   +      throw new InvalidArgumentException(sprintf('The "uid" argument needs to be a number, but it is "%s".', $uid));
   

   s/a number/an integer/

--- /dev/null
+++ b/core/tests/Drupal/TestSite/Commands/TestSiteUserLoginCommand.php

This only does the login bit from the issue title.

Either the patch still needs a logout command too, or the issue title and summary need to be updated.

This only does the login bit from the issue title.

We don't need PHP command for logging out. There's a Nightwatch command for doing that. 🙌

Ok!

Also.

core/tests/Drupal/Nightwatch/Commands/drupalLogin.js
core/tests/Drupal/Nightwatch/Commands/drupalLogout.js

Fixed #28 and #26.

Talked to @lauriii, I didn't realize there were both Nightwatch and Drupal commands (because I was asked to review the PHP side). I think this issue title covers it all.

1. +++ b/core/tests/Drupal/TestSite/Commands/TestSiteUserLoginCommand.php
   @@ -26,29 +26,13 @@ class TestSiteUserLoginCommand extends Command {
   +      ->addArgument('uid', InputArgument::OPTIONAL, 'The ID of the user for whom the link will be generated', 1)
   

   This is still optional. Why?

2. +++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
   @@ -254,6 +250,50 @@ public function testTearDownDbPrefixValidation() {
   +    $this->assertSame(0, $process->getExitCode());
   ...
   +    $this->assertSame(1, $process->getExitCode());
   

   Shouldn't we also assert the output?

   And in the latter case: shouldn't we perform a HTTP request to the given URL and verify that the response contains a Set-Cookie header with the Drupal session cookie?

- fixes linting and jsdoc issues.

... coming from the meeting in slack... where there was a call for review.

FWIW after a slow visual scan of the patch all the code patterns employed here...look straight out of the text book...

nothing that leaps out as questionable.

My feeling - A good addition to the project

So +1 from me.

Addresses #36.

+++ b/core/tests/Drupal/TestSite/Commands/TestSiteInstallCommand.php
@@ -102,6 +102,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
+        'site_path' => $this->siteDirectory,

+++ b/core/tests/Drupal/Tests/Scripts/TestSiteApplicationTest.php
@@ -254,6 +250,61 @@ public function testTearDownDbPrefixValidation() {
+    $this->assertSame('sites/simpletest/' . str_replace('test', '', $db_prefix), $site_path);

FWIW, this assert is checking the change in install command.

Writing a change record.

Created the following change record https://www.drupal.org/node/2986276. Feel free to improve it.

Committed and pushed 0083ba97d7 to 8.7.x and c5782b95a2 to 8.6.x. Thanks!

Published the change record as well. Thanks! for the commit.