Leverage JS for JS testing (using nightwatch)

I hope this will be a useful comment,

I agree with

It doesn't make sense to use PHP to test JS

While we evaluate this further can I ask a question about nightwatch's abilities....

Over in #2862510: Convert system/tests/src/Ajax to JavascriptTestBase

we have been looking for a good pattern to use

Is it possible in nightwatch to control the browser to submit invalid form requests, to simulate a bad actor and attack the site.

it is something Mink does not entertain.

In short how would we convert this code segment from FormValuesTest::testSimpleAjaxForm()

    // We don't need to check for the X-Drupal-Ajax-Token header with these
    // invalid requests.
    $this->assertAjaxHeader = FALSE;
    foreach (['null', 'empty', 'nonexistent'] as $key) {
      $element_name = 'select_' . $key . '_callback';
      $edit = [
        $element_name => 'red',
      ];
      $commands = $this->drupalPostAjaxForm('ajax_forms_test_get_form', $edit, $element_name);
      $this->assertResponse(500);
    }

I'm still not a 100% sure on this but am very interested to see a couple of representative tests converted and working.

My doubts are as follows:

1. We force the current test writers to learn another testing framework and we're seriously lacking people who are interested in writing tests already.
2. Jumping from BrowserTestBase to JavascriptTestBase is quite easy right now.
3. We split the functional testing into 2 separate frameworks. Upgrading an test to a JavaScript test will be more difficult and I really don't like the split in functional testing.
4. Will 'the people' really come if we have another testing framework?
5. Is it really possible to work on JavaScript alone in Drupal core or is it more of a mix-n-match with the php backend?

The main pro's I see are:

1. Same testing framework for functional javascript testing and javascript unit testing.
2. Testing in the same language as the code itself.

In short my programming profile so my doubts can be put into perspective:
I'm mainly a Drupal backend PHP programmer and am one of the PHPUnit initiative leads. I have quite a lot of experience with building a JavaScript web-app on top of Drupal (I have built a complex backbone based progressive decoupled web-app). I'm certainly not protective of the JavascriptTestBase and am open to any alternative that integrates well with Drupal and the rest of the testing needs.

I would love to see a something converted that uses drag-n-drop and a dialog or contextual links though, these are the difficult parts to test in the current JavascriptTestBase.

Patch no longer applies... I have tried a reroll but it is very difficult to untangle all the changes to yarn.lock

#11 is an I interesting perspective...

I am in the process of forming an opinion

For what it is worth ---

1) This stuff was discussed here.

https://www.lullabot.com/podcasts/drupalizeme-podcast/modernizing-javasc...

2) nightwatch's documentation looks very good. and the pattern in the example look just like our Mink code.

http://nightwatchjs.org/

3) "More example of difficult to solve problems" ... would be a good way to go... I will help out where I can :)

I have similar thoughts with @michielnugter.

In most of the cases, we actually testing the UI functionality, not testing the JS code. On another side, PHP developers can't write JS also. You make changes to PHP & JS together. To understand basic PHP is a must in Drupal Core Development.

Also, many of tests get the dynamic data from DB and PHP.

A real case study:
#2725255: Unfiltered data in "Allowed HTML tags"
testJSInjection: ideally, it should provide by PHP Developers. (PHP Developer creates the UI. TDD.)

testParserError: should be a JS UnitTest (mocha)

What discourage our developer to contribute to tests:

[Slightly Off-topic] I think one of the main problems is CORE development always testbots & Core committers first, not contributors first. It squeezed our patience to last minute until you give up. Bug A creates Bug B, Bug B creates C. And Bug E blocked A bring into CORE..etc

- Slow test.
It spent few mins on a simple test in my local env. It easily wasted an hour or so on a random bug.
#2747075: [meta] Improve WebTestCase / BrowserTestBase performance by 50%

- Hard to debug.
Missing visual way to debug:
https://twitter.com/webdriverio/status/806911722682544128
#2857957: Explore a way to provide interactive shell for tests (or use Psysh)

Anyway, what test engines was rejected in the offline meetings?

@dawehner Thanks for the IS update!

Is it possible in nightwatch to control the browser to submit invalid form requests, to simulate a bad actor and attack the site.

I think this should be moved to BrowserTestBase and simulate the request directly, it should be possible there. Doing this in a real-browser env just seems wrong. Still wrong in BrowserTestBase but at least a little less wrong ;)

I think the fact that people call out to the container in the middle of the test is way worse than people actually estimate. If you use the container in the middle of the test, you act on a different state of the world, than what is available via HTTP in the browser.

Mostly it's done to quickly change a setting instead of clicking through the interface. It's a time-saver and makes the test more readable. Should we completely stop doing this? I agree it makes sense to only use the UI but it would mean a significant increase in test-complexity.

Testing from JS would make the impossible.

Educating people about other technologies aren't a bad thing. People are maybe scared, but I believe career wise its never a bad idea to learn things.

Oh sure, completely agree! I actually came to Drupal to learn stuff and actually like learning about this a lot. I just have doubts about this switch and if it will really improve Drupal in general. To be clear: doubts don't mean I'm against it, I just see some problems / considerations.

Do you have a partical example of a test in mind? It would be great to list 1-2 examples in the issue summary and provide an example of them in the test.

Drag-n-drop: \Drupal\Tests\book\FunctionalJavascript\BookJavascriptTest.php
Dialog: \Drupal\Tests\views_ui\FunctionalJavascript\FilterCriteriaTest
Dialog:
Complex test: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest

I updated the IS on this.

Nightwatch makes it easy to open up a real browser to see what the test is doing

The issue is open to move to Webdriver making this possible for the current phpunit JavascriptTestBase as well. Does NightwatchJS offer more?

Of course when we would switch to whatever new technology, we would not just burn everything else down.

Do you mean to support both NightwatchJS and JavascriptTestBase together?

To me: if we're going to switch we should deprecate JavaScriptTestBase and move core to the new testing framework.

Im bumping the priority of this.

PhantomJS is broken, and we didnt really run into it until we tried to repurpose some extra hardware in our racks for some testbots. Basically it freezes because the underlying networking subsystem is broken. (https://github.com/ariya/phantomjs/issues/14286) The only options to make it work at this point are upgrade it to a fully unsupported 2.5.0 beta version, and *hope* that fixes the problems, or get rid of it once and for all and convert everything to *not* use phantomjs.

Of course when we would switch to whatever new technology, we would not just burn everything else down.

Can we burn phantom js just a little?

@Mixologic

I think the road for this is still a bit too long to help you out in short term. Also as stated by @dawehner in #16 this issue would not mean removing the current form of JavascriptTestBase in the short run, we'll have to keep supporting JavascriptTestBase at least until 9.x.

I think the most efficient way of getting rid of PhantomJS for the test-bot is getting this issue in: #2775653: JavascriptTests with webDriver.

That's about switching to Webdriver for JavascriptTestBase, after that gets in we could do a follow-up to change the browser to chrome headless?

I'm in favor of resetting the priority to normal here and upgrading the priority on the mentioned issue as this is really a huge change that really can't use a lot of pressure to get in, if we're going to switch we want to do it right.

Feel free to disagree ;)

Yes, perhaps this is not the issue that is the shortest path to gutting phantomjs. As long as there is a path forward that has some way that the testbots do not have to have that as an option, thats great (it certainly doesnt have to be this issue).

Debugging those hard issues have lead to most probably hundreds of hours of hard to debug tests.

Given the PHP script would bootstrap Drupal to some respect it would be more safe to do the necessary changes. Do you think this makes sense?

Nice to see this change forces us to think about how to test specific situations :)

Clicking through the UI won't make the tests better readable but it may make them better understandable.

Though if we separate environment preparation from the actual tests (could be a get call to a specific URL or a method on the test class itself) sounds like a good idea. It makes documentation better possible and more clear that the purpose is environment preparation.

Well, this is what happened with phpunit tests, web tests and browsers tests as well. Unit tests got introduced quite early (2013) but it took time to be leveraged. The same thing happened with BrowserTestBase (which is 2 years old now). Future planning is part of big opensource projects :)

For sure we have to keep support for JavascriptTestBase but if we change I would like all current tests converted to use the best framework. We don't have that many tests currently so it's still doable.

👍

Next Step, probably to consider if we should use Snapshot for testing.

What is Snapshot?
https://facebook.github.io/jest/docs/en/snapshot-testing.html

DONE: #2912788: [JS] Introduce Snapshot testing into CORE

I'm not sure it matters what testing framework we use as long as:

• It can be integrated into DrupalCI
• The JS maintainers agree

I have not used nightwatch but regardless I'm +1 because some JS based testing is better than none and nightwatch appears to have consensus. If the JS maintainers all agree I think we should go forward with this.

I'd like to quickly chime in here and mention TestCafe as an alternative to Nightwatch. We've recently assessed and switched to TestCafe because of its many benefits compared to Nightwatch. That being said, we've happily used Nightwatch in the past. I don't want to derail this discussion and Nightwatch is certainly better than simply having nothing of the sort. Anyway's, I'd like to highlight a few of the benefits that we saw in TestCafe:

1. Minimal setup. It doesn't require you to install WebDriver / Selenium. It just works with a simple "yarn add testcafe --dev". Done. You can then use it with any browser that you've installed locally.
2. You can write your tests with ES6+.
3. Asynchronous operations are much nicer to test. Waiting is built-in and you can use async/await for that.
4. It seems like TestCafe is going to become to modern successor of Nightwatch.
5. It has some cool features that are missing in Nightwatch to my knowledge. E.g. you can use a virtual mouse cursor in your tests.

There's more. But these are some of the most compelling reasons that sold us.

As I said, I don't want to derail or postpone this issue. Just wanted to mention it and was wondering if anyone has given it a try before.

Just that you know, TestCafe is a service (https://testcafe.devexpress.com/), but also is an open source library (https://github.com/DevExpress/testcafe). @fubhy talked about the library.

Escalating to major, since this is a blocker for JS testing.

I discussed #2775653 with @dawehner and @laurii today and we came to the conclusion it isn't a blocker for this issue

Regarding TestCafe et al - @drpal, @dawehner, @laurii and myself looked at this and other options today together (directly using Puppeteer + an assertion library of our choice). My thoughts - whilst TestCafe seems very interesting, I believe it's too green at this point for us to depend on it. The other thing which makes me slightly nervous about it is that it uses a URL-rewriting proxy, which is what Selenium used in v1 before moving away from it because of various issues. Puppeteer was an interesting option, but I think it's nice to leave ourselves the option of testing across multiple browsers.

So still +1 for Nightwatch from me

Thanks for #37. Regarding TestCafe's pros from #31:

You can write your tests with ES6+.

Would we not be able to with Nightwatch?

Asynchronous operations are much nicer to test. Waiting is built-in and you can use async/await for that.

How would waiting for async operations to complete be done with Nightwatch?

It seems like TestCafe is going to become to modern successor of Nightwatch.

What do you base that on? That claim does not yet seem to be supported by usage trends.

I don't want to derail this discussion and Nightwatch is certainly better than simply having nothing of the sort.

Given that and #37, I'm +1 to proceeding with Nightwatch for now, so removing the "Needs framework manager review" tag. However, I'm still curious about the above questions, or any other potential challenges with Nightwatch that people want to surface.

ES6+,
I think #31 meant the built-in features rather than config babel yourself.

Asynchronous,
In nightwatch, you have to write another custom wrapper inside executeAsync for async, right?
http://nightwatchjs.org/api#executeAsync

In testCafe, it's more elegant, (diff test engine but same thing, it describes the diff better) :
https://github.com/avajs/eslint-plugin-ava/blob/master/docs/rules/prefer...

Ask a stupid question...

1. Do we write JS UnitTest for Core?
2. If Yes, How to test React? Any plan?
3. If Yes, do we need to perform BENCHMARK between different engines against the REAL Drupal env (ANd on a single machine)? (Performance is a matter to encourage the contributions!)

One of the key things that has unblocked #2775653: JavascriptTests with webDriver is that there is now a webdriver implementation on the testbots (chromedriver) #2857788: Patch the testbot to start both chrome in webdriver mode and phantomjs in non webdriver mode. Currently the hostname of that container is determined at runtime, and is setting an environment variable in the run-tests.sh command to let the testrunner know where it is. So if/when we implement something, we've already got the chromedriver part working on the testbots.

When I looked into testcafe, this thread had some red flags in it that I think we should actively avoid:
https://testcafe-discuss.devexpress.com/t/why-not-use-selenium/47/2

Specifically, when they say "Selenium" they mean "the W3C standard webdriver API" - there seems to be a *lot* of value in leveraging the webdriver api, and it just feels like they are optimizing for some of the less important use cases, i.e. 'we wanted to simplify setting up the test environment. ' -> afaik, setting up a local chromedriver env to work with nightwatch isn't terribly difficult. And to second what Justafish pointed out earlier, the URL rewriting proxy is a brittle link and bound to introduce subtle errors.

The webdriver api gives us the ability to add a geckodriver, or edgedriver container without needing to do a whole lot to our tests.

I dont think that testcafe offers the right type of solution for testing a codebase that is intended to be run across a wide spectrum of installations.

So, if we do decide to proceed with nightwatch.js, Can somebody please open an issue over at https://drupal.org/project/drupalci_testbot with what steps the ci system should take to ensure that nightwatch is installed? maybe thats this issue? #2874028: Create a Yarn Build step

And also open an issue to let us know *how* those tests are going to run., what kind of output they produce, and where. (i.e. are we going to have run-tests.sh kick off nightwatch tests, and take all of its junit formatted output and put it back into the same result set as everything else, or are we planning on producing output in parallel, with a different runner?) edit: I just looked and saw that [#12373756-38] is exactly this

I am motivated to reroll...

This may blow up horribly...the patch is so old.
when this comes back green I will convert to es6

Fair enough.

This has been worked on by the JavaScript Initiative and is ready for a review here on d.o 🎉
It requires the patch from 2926633 to be applied first.

Here is a branch which contains both this work and the work from 292663:
https://github.com/jsdrupal/nightwatch/pull/1

And here is just this patch:
https://github.com/jsdrupal/nightwatch/compare/8.6.x...nightwatch-no-ins...
https://github.com/jsdrupal/nightwatch/compare/8.6.x...nightwatch-no-ins...

We're more than happy to take code reviews on GitHub (or PRs to the nightwatch branch!) if that's easier for anyone reviewing this 👍

This has been worked on by several people in the JavaScript initiative: justafish, dawehner, alexpott, drpal, mglaman, and mixologic

Adding issue credit using list in #48

I just followed the steps in the README and it works!! Pretty sweet!

First run through:
when installing I get a : " warning Lockfile has incorrect entry for "glob@^7.1.2". Ignoring it."

The dotenv docs need some love I feel:

1. +++ b/core/.env.example
   @@ -0,0 +1,57 @@
   +# Provide this value if you want to use a stand-alone web server instead of
   +# PHP's built-in server. Do not include a trailing slash.
   +#DRUPAL_TEST_BASE_URL=http://localhost
   

   'this value' and then we have 3 values. I'm assuming DRUPAL_TEST_BASE_URL but lets be more specific then 'this value'

2. +++ b/core/.env.example
   @@ -0,0 +1,57 @@
   +#DRUPAL_TEST_WEBSERVER_USER=www-data
   

   This could use some explanation as to it's use

3. +++ b/core/.env.example
   @@ -0,0 +1,57 @@
   +DRUPAL_TEST_DB_URL=sqlite://localhost/sites/default/files/db.sqlite
   

   This could use some explanation as to it's use and maybe directions to use alternatives to sqlite (if we want to), or else just say you need to leave this alone?

4. +++ b/core/.env.example
   @@ -0,0 +1,57 @@
   +# If Chromedriver is running as a service elsewhere, set it here.
   +DRUPAL_TEST_WEBDRIVER_HOSTNAME=localhost
   +DRUPAL_TEST_WEBDRIVER_PORT=9515
   

   This makes it sound like these settings ar optional, which they are not. So I'd add something like 'When using DRUPAL_TEST_CHROMEDRIVER_AUTOSTART leave this at the default settings.'

5. +++ b/core/.env.example
   @@ -0,0 +1,57 @@
   +# Use your own webdriver or chromedriver setup.
   

   Use => use

6. +++ b/core/.env.example
   @@ -0,0 +1,57 @@
   +DRUPAL_NIGHTWATCH_OUTPUT=reports/nightwatch
   

   this could use a little explanation block like the others have. 'Nightwatch generates output files. Use this to specify the location where these files need to be stored. The default location is ignored by git, if you modify the location you will probably want to add this location to your .gitignore.', something like that.

7. +++ b/core/.env.example
   @@ -0,0 +1,57 @@
   +# This uses minimatch syntax. Separate folders with a comma.
   +DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES=node_modules/,vendor/,.git/
   

   This could use a little explanation as to what it does besides what syntax to use.

When enabling ESLint in PHPStorm, it had something to say about a lot of the .js files getting added here, so might be worth running that to clean some stuff up.

Some stuff I see that ESLint won't pick up:

1. +++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
   @@ -0,0 +1,39 @@
   +exports.command = function installDrupal(setupClass = '', callback) {
   

   it's a function so we probably need a docblock

2. +++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
   @@ -0,0 +1,39 @@
   +        // Colons needs to be URL encoded to be valid.
   

   needs => need

3. +++ b/core/tests/Drupal/Nightwatch/Commands/relativeURL.js
   @@ -0,0 +1,20 @@
   +exports.command = function relativeURL(pathname, callback) {
   

   missing @param for callback

4. +++ b/core/tests/Drupal/Nightwatch/Commands/uninstallDrupal.js
   @@ -0,0 +1,22 @@
   +exports.command = function uninstallDrupal(dbPrefix = '', callback) {
   

   it's a function so we probably want a docblock

5. +++ b/core/tests/Drupal/Nightwatch/Commands/uninstallDrupal.js
   @@ -0,0 +1,22 @@
   +  // Nightwatch doesn't like it when no actions are added in command file.
   +  const dbOption = process.env.DRUPAL_TEST_DB_URL.length > 0 ? `--db_url ${process.env.DRUPAL_TEST_DB_URL}` : '';
   

   That comment doesn't seem relevant here

6. +++ b/core/tests/Drupal/Nightwatch/Commands/uninstallDrupal.js
   --- /dev/null
   +++ b/core/tests/Drupal/Nightwatch/Tests/index.js
   

   Already saw that this was mentioned on the slack channel, but lets rename this to something other then index.js

7. +++ b/core/tests/Drupal/Nightwatch/nightwatch.conf.js
   @@ -0,0 +1,73 @@
   +      // This is necessary to avoid infinite loops with zero-width matches
   

   missing colon at the end.

Thanks @Lendude! PR updated and new patch attached (mostly fixed by @dawehner) https://github.com/jsdrupal/nightwatch/pull/1

Interdiff: https://github.com/jsdrupal/nightwatch/compare/2a1d4ceee9a2b65c453610ce0...

Sorry, wrong branch :)

This patch update incorporates @Lendude's feedback as well as work @dawehner did on simplifying the install/uninstall commands within Nightwatch.

Interdiff: https://github.com/jsdrupal/nightwatch/compare/4fde987...nightwatch-no-i...

---
https://github.com/jsdrupal/nightwatch/pull/1
https://github.com/jsdrupal/nightwatch/compare/8.6.x...nightwatch-no-ins...
https://github.com/jsdrupal/nightwatch/compare/8.6.x...nightwatch-no-ins...

+++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
@@ -0,0 +1,57 @@
+    setupClass = setupClass ? `--setup_class ${setupClass.replace(/\\\\/g, '\\\\\\\\')}` : '';

This is now not escaping right, the setup class winds up being DrupalTestSiteTestSiteInstallTestScript and so the test fails to run now (for me locally at least), it works when reverted back to setupClass.replace(/\\/g, '\\\\') (also a little easier on the eyes :-)

The rest of the changes look good.

+++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
@@ -0,0 +1,57 @@
+        name: 'SIMPLETEST_USER_AGENT',

We should add a comment why are we adding cookie prefixed simpletest. Maybe @todo with issue ID to remove this as well. When simpletest is gone.

Drupalci is now noticing that the tests are failing, but the 'build successful' part of things is not doing what it is supposed to do. Im working on improving that.

1. +++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
   @@ -0,0 +1,57 @@
   +  // Allow to skip the setupClass.
   

   setupFile now :) Also might be better as Allow the setupFile argument to be omitted

2. +++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
   @@ -0,0 +1,57 @@
   +    // Single slash is replaced with 2 slashes because it will get printed on the command line,
   +    // which will be escaped again by the PHP script.
   

   I think this comment is now obsolete

3. +++ b/core/tests/Drupal/Nightwatch/Commands/relativeURL.js
   @@ -0,0 +1,22 @@
   +
   

   Why a new line here? Is it enforced by eslint?

4. +++ b/core/tests/Drupal/Nightwatch/Commands/uninstallDrupal.js
   @@ -0,0 +1,38 @@
   +  // Nightwatch doesn't like it when no actions are added in command file.
   

   I'm not sure this comment is needed here.

Forgot to credit myself from the list in #48.

We're going to need a change record for this.

Interdiff: https://github.com/jsdrupal/nightwatch/compare/3d2c2c1e4ecd1629f15dfcb17...

The permission problem we're having is due to a couple of tests in drupal core that deliberately set the directory permissions to 0000 as part of a test, and that one directory then ends up lingering around:

http://cgit.drupalcode.org/drupal/tree/core/modules/file/src/Tests/SaveU...

I've put a hacky fix into drupalci, but ultimately, Im pretty sure that this shouldnt be scanning the whole filesystem, as it's bound to run into other issues elsewhere.

I've started the test and its rerunning now.. results in #76 will reflect the new situation soon.

Requeued, because, as my other experiments have taught me, FunctionalJavascript tests are kinda fragile.

There. we. Go.

Just to point out that the test which checks test-site.php is both buggy and not being run by the testbot: #2962157: TestSiteApplicationTest requires a database despite being a unit test

@Mixologic did you end up excluding the directory with DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES in .env?

Nits:

+++ b/core/.env.example
@@ -0,0 +1,66 @@
+DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES=node_modules/,vendor/,.git/,sites/*/files

.idea/ maybe?

+++ b/core/tests/Drupal/Nightwatch/Commands/uninstallDrupal.js
@@ -0,0 +1,37 @@
+ *   (optional_ The database prefix, which should be teared down.

_ => )
teared down => torn down

Not nit:
I tried adding a second test in a different spot to test if the discovery was working. In my case I went to core/modules/views/tests/src/Nightwatch/Tests and copied the example test there. Changed the name of the test but that was it.

Ran nightwatch, discovery was fine, but the test would hang on the second test all the time (which in this case was the original exampleTest). After a couple of minutes I would kill it, second time round, same result.
After some more messing around I've seemed to have killed everything:

Error retrieving a new session from the selenium server

Connection refused! Is selenium server started?
{ Error: connect ETIMEDOUT 127.0.0.1:9515
    at Object._errnoException (util.js:1022:11)
    at _exceptionWithHostPort (util.js:1044:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1198:14)
  code: 'ETIMEDOUT',
  errno: 'ETIMEDOUT',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 9515 }

error An unexpected error occurred: "Command failed.
Exit code: 1
Command: sh
Arguments: -c cross-env BABEL_ENV=development node -r dotenv-safe/config -r babel-register ./node_modules/.bin/nightwatch --config ./tests/Drupal/Nightwatch/nightwatch.conf.js
Directory: /Applications/MAMP/htdocs/d8/drupal/core
Output:
".
info If you think this is a bug, please open a bug report with the information provided in "/Applications/MAMP/htdocs/d8/drupal/core/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
⏎                                       

But I wouldn't expect problems when having two tests, so that needs a look I think.

   - Test page two (3m 52s)
   Timed out while waiting for element <body> to be present for 1000 milliseconds.  - expected "visible" but got: "not found"
       at Object.Test page two (/Applications/MAMP/htdocs/d8/drupal/core/tests/Drupal/Nightwatch/Tests/exampleTwoTest.js:12:8)
       at _combinedTickCallback (internal/process/next_tick.js:131:7)

Rebooted, tried again, hangs again on the second test. I’m just literally following the README with a completely unchanged .env file.

I see that chrome goes to localhost:8888 and then keeps waiting. The 'timed out' error above only appeared after I stopped chrome, so the 'waitFor' didn't seem to trigger either after 1000ms.

@lendude: if its timing out on ETIMEDOUT 127.0.0.1:9515, thats nightwatch trying to connect to chromedriver's default port of 9515. Are you running selenium standalone or chromedriver? (do the instructions tell people that its necessary?)

@justafish: No, I didn't use DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES in .env?, I just did a recursive chmod on the sites directory to avoid the permissions issue. What should I set that to? 'sites' dir?

@justafish: re #83: I just re-reviewed what drupalci is really doing, and it reads in the .env.example file and it preserves the DRUPAL_NIGHTWATCH_IGNORE_DIRECTORIES.

The issue is that its currently set to node_modules/,vendor/,.git/,sites/*/files, but the folder that it was finding was in /var/www/html/sites/simpletest/66530328/files/dir/tests, so I think we might need to adjust
sites/*/files to be sites/**/files so it ignores those oddball test folders.

Nice work on minimizing the steps required for being able to run JavaScript tests. I was able to run these tests in less than five minutes!

1. +++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
   @@ -0,0 +1,55 @@
   +    const matches = process.env.DRUPAL_TEST_BASE_URL.match(/^https?\:\/\/([^\/?#]+)(?:[\/?#]|$)/i);
   ...
   +    const path = matches[2];
   

   This regex doesn't capture the path.

2. +++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
   @@ -0,0 +1,55 @@
   +    this.assert.fail(error);
   

   What does this do? I couldn't find any mentions of this in the documentation.

   What is the current behavior in case of an error? Does this.assert.fail() prevent the tests from continuing? Do we attempt to tear down even if the installation failed?

3. +++ b/core/tests/Drupal/Nightwatch/Commands/uninstallDrupal.js
   @@ -0,0 +1,37 @@
   + *   (optional_ The database prefix, which should be teared down.
   

   s/(optional_/(optional)

4. +++ b/core/tests/README.md
   @@ -102,3 +102,27 @@ export SIMPLETEST_BASE_URL='http://d8.dev'
   +## Nightwatch tests
   

   Can we document how to run a single test?

assert.fail is documented here and here - Nightwatch extends node's built in assertion library. It'll fail and stop the tests if used.

Interdiff: https://github.com/jsdrupal/nightwatch/pull/29/files/7c7b305f5189b0affc4...

Re-roll into a unified patch

Addresses feedback from @lauriii in Slack

interdiff: https://github.com/jsdrupal/nightwatch/pull/29/files/f14c7fe06fd241eed7b...

+++ b/core/tests/README.md
@@ -102,3 +102,33 @@ export SIMPLETEST_BASE_URL='http://d8.dev'
+- Install [Node.js](https://nodejs.org/en/download/) and [yarn](https://yarnpkg.com/en/docs/install). The versions required are specificed inside core/package.json in the `engines` field

Can it support a minimum engine version or exact version? I was able to get it working with the exact versions, didn't try latest versions.

- Adds more specific engine requirements (latest node 8 security release and latest version of yarn)
- Removes .nvmrc as this will be soon be pulled from package.json, and we don't want to keep it updating versions in 2 places (https://github.com/creationix/nvm/pull/1535)
- Fixes the hang encountered after using the uninstall command
- Refresh yarn lockfile

Interdiff: https://github.com/jsdrupal/nightwatch/pull/35/files https://patch-diff.githubusercontent.com/raw/jsdrupal/nightwatch/pull/35...

"Adds more specific engine requirements (latest node 8 security release and latest version of yarn)"
I'm not sure if this was added because of my comment in #93, but I only meant to ask a question, not suggest any change. What I meant to ask was: Can someone use a newer engine version or does it only support the version specified?

I updated drupalci to have the latest versions of yarn and nodejs, and the php7/mysql5.5 environment is passing. Im not going to retest all the other environments, as Im not sure what those were attempting to establish.

Can we please have a review only patch as well to make reviewers life easier?

@jibran not sure what you want from a review only patch since everything needs to be reviewed - yes you can ignore some parts of the yarn.lock but the added dependencies should be reviewed along with everything else.

Yeah, that's what I meant.

I used nightwatch branch from https://github.com/jsdrupal/nightwatch merged it with 8.6.x and created the patch and didn't include the yarn.lock file.

Updates patch to fix the tests hanging and never exiting in some circumstances, and a command to log out the browser console to a JSON file.

Interdiff from #94: https://github.com/jsdrupal/nightwatch/pull/36.diff

Fixes test failure where currentTest.name isn't populated in Nightwatch if there's only 1 test step.

Interdiff: https://patch-diff.githubusercontent.com/raw/jsdrupal/nightwatch/pull/39...

Still same result. Here is the interdiff to #105 that I'm using.

lendude@DudePro /A/M/h/d/d/core> yarn test:nightwatch
yarn run v1.6.0
$ cross-env BABEL_ENV=development node -r dotenv-safe/config -r babel-register ./node_modules/.bin/nightwatch --config ./tests/Drupal/Nightwatch/nightwatch.conf.js
Starting ChromeDriver 2.38.552518 (183d19265345f54ce39cbb94cf81ba5f15905011) on port 9515
Only local connections are allowed.

[Example Consecutive Test] Test Suite
=========================================

Running:  Test consecutive tests
 ✔ Element <body> was visible after 27 milliseconds.
 ✔ Testing if element <body> contains text: "Test page text".

OK. 2 assertions passed. (22.896s)

[Example Test] Test Suite
=============================

Running:  Test page
 ✖ Timed out while waiting for element <body> to be present for 1000 milliseconds.  - expected "visible" but got: "not found"
    at Object.Test page (/Applications/MAMP/htdocs/d8/drupal/core/tests/Drupal/Nightwatch/Tests/exampleTest.js:14:8)
    at _combinedTickCallback (internal/process/next_tick.js:131:7)


FAILED:  1 assertions failed (1m 13s / 73774ms)

 _________________________________________________

 TEST FAILURE:  1 assertions failed, 2 passed. (1m 37s)

 ✖ exampleTest

   - Test page (1m 13s)
   Timed out while waiting for element <body> to be present for 1000 milliseconds.  - expected "visible" but got: "not found"
       at Object.Test page (/Applications/MAMP/htdocs/d8/drupal/core/tests/Drupal/Nightwatch/Tests/exampleTest.js:14:8)
       at _combinedTickCallback (internal/process/next_tick.js:131:7)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
⏎ 

As you can see the first test runs fine, the time out in the second test only occurs once I close the Chrome window that the test spawned. I've let it run for 5 minutes, but nothing happens, just an empty page that points to data:,

woot!

Ahhhh the old d.o issue queue rubber duck doing its magic, while writing this I got it to run. MAMP PRO is getting in the way. Stopping the services there makes the tests pass. Why this would impact only the second test, not a clue!

Removes auto-starting of the web server as it's causing problems for me too with the second test @lendude provided. Seems fine when I start it myself and it doesn't seem unreasonable to request the user start the webserver.

interdiff: https://github.com/jsdrupal/nightwatch/pull/29/files/9bafc5faec44d3983f5...

1. +++ b/core/.env.example
   @@ -0,0 +1,71 @@
   +# don't already have one running, e.g. Apache, you can use PHP's built-in web
   +# server by running:
   

   maybe add 'by running the following command in your Drupal root folder'. Cause the command works fine when .ht.router.php can't be found, the tests just fail with very verbose errors.

2. +++ b/core/.env.example
   @@ -0,0 +1,71 @@
   +# If you have Drupal installed into a docroot folder, you can use the following
   +# setup to add integration tests for your project outside of ones specifically
   +# for custom modules/themes/profiles.
   +# docroot/
   +#   core/
   +# tests/
   +#   Nightwatch/
   +# and then set DRUPAL_NIGHTWATCH_SEARCH_DIRECTORY=../
   

   It's unclear to me from the documentation what this does and how this would work. Is it just weird indenting?

3. +++ b/core/package.json
   @@ -20,37 +25,59 @@
   +    "dotenv-safe": "4",
   

   why just "4", seems more loose then the other packages

4. +++ b/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
   @@ -0,0 +1,46 @@
   +  // Nightwatch doesn't like it when no actions are added in a command file.
   +  this.pause(1);
   

   It's annoying that we need to do this every time, is there an upstream issue for this, and if so, can we link to this somewhere?

5. +++ b/core/tests/Drupal/Nightwatch/Commands/logAndEnd.js
   @@ -0,0 +1,22 @@
   + * @param {function} callback
   ...
   +exports.command = function endAndLog({ onlyOnError = true }, callback) {
   

   Uhhh this is called as logAndEnd() in the rest of the code. How does that work??
   Also not the most descriptive name ever, logErrorsAndEndBrowserSession maybe?

6. +++ b/core/tests/Drupal/Nightwatch/Commands/logAndEnd.js
   @@ -0,0 +1,22 @@
   + *   A callback which will be called.
   

   We are missing a @param here.

7. +++ b/core/tests/Drupal/Nightwatch/Commands/uninstallDrupal.js
   @@ -0,0 +1,40 @@
   + * @param {string} dbPrefix
   + *   (optional) The database prefix, which should be torn down.
   

   The param doesn't exist any more

All feedback addressed, except for logAndEnd which I kept as it is because it's analogous to the built-in "end" command http://nightwatchjs.org/api/end.html

Interdiff: https://github.com/jsdrupal/nightwatch/pull/42/files

+++ b/core/.env.example
@@ -0,0 +1,77 @@
+# .
+# ├── docroot
+# │   ├── core
+# ├── tests
+# │   ├── Nightwatch
+# │   │   ├── Tests
+# │   │   │   ├── myTest.js

That is really clear! Great improvement.

except for logAndEnd which I kept as it is because it's analogous to the built-in "end" command

that's fair. Good reason to keep it that way.

---------------------------------------------

Ran the whole setup again, ran tests against both the PHP build in web server and Apache (using MAMP) and both worked without issue.

I have no idea if the set up instructions are valid or working in any other OS then OSX, did anyone verify that? I see that a number of people ran this successfully, but don't know on what systems they did so.

But this looks great from my end, all feedback has been addressed, RTBC from me.

Crediting @michielnugter and @droplet for discussions on the overall state of JS testing.
Crediting @lauriii for reviews and discussions in slack
Crediting @effulgentsia for framework manager review.

Some coding standards issues:

/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Nightwatch/Commands/installDrupal.js
  37:3  error  Expected space(s) after "catch"  keyword-spacing

✖ 1 problem (1 error, 0 warnings)


/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Nightwatch/Commands/uninstallDrupal.js
  17:77  error  Block must not be padded by blank lines  padded-blocks

✖ 1 problem (1 error, 0 warnings)


/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Nightwatch/Tests/exampleTest.js
  3:3   error  Expected method shorthand                  object-shorthand
  3:19  error  Missing space before function parentheses  space-before-function-paren
  7:3   error  Expected method shorthand                  object-shorthand
  7:18  error  Missing space before function parentheses  space-before-function-paren

✖ 4 problems (4 errors, 0 warnings)


/Volumes/devdisk/dev/sites/drupal8alt.dev/core/tests/Drupal/Nightwatch/nightwatch.conf.js
  11:73  error  Multiple spaces found before '||'                                                    no-multi-spaces
  28:45  error  Expected parentheses around arrow function argument having a body with curly braces  arrow-parens
  38:39  error  Expected parentheses around arrow function argument having a body with curly braces  arrow-parens
  43:33  error  ["Tests"] is better written in dot notation                                          dot-notation
  45:42  error  ["Commands"] is better written in dot notation                                       dot-notation
  46:44  error  ["Assertions"] is better written in dot notation                                     dot-notation
  70:33  error  Missing trailing comma                                                               comma-dangle

✖ 7 problems (7 errors, 0 warnings)

Patch attached addresses them + @lauriii's comment on https://github.com/jsdrupal/nightwatch/pull/42/files

Committed aeb5733 and pushed to 8.6.x. Thanks!

diff --git a/core/yarn.lock b/core/yarn.lock
index 02fb0e3786..ed069d98a7 100644
--- a/core/yarn.lock
+++ b/core/yarn.lock
@@ -1401,7 +1401,7 @@ dot-prop@^4.1.1:
   dependencies:
     is-obj "^1.0.0"
 
-dotenv-safe@5.0.1:
+dotenv-safe@^5.0.1:
   version "5.0.1"
   resolved "https://registry.yarnpkg.com/dotenv-safe/-/dotenv-safe-5.0.1.tgz#8c4a79b8978fd4271b3d8ef17be2b2f04588af71"
   dependencies:

Forgot to update the lock file in #112. Updated it on commit.

🚀🎉