Add test coverage for machine-name.js

Updated the issue summary a bit.

A javascript meta issue for the whole of core would probably be too big in scope.

I set up a meta issue just for Views #2752931: [meta] Implement Javascript testing for Views and the Views UI so that is somewhat more limited in scope and pretty maintainable. There might be other meta's like that floating around.

@michielnugter,

There're some issues needed JSTests:

https://www.drupal.org/project/issues/search?projects=Drupal+core&projec...

Huge one:
#2809161: Convert Javascript/AJAX testing to use JavascriptTestBase

I very enjoy to see your detail reports of each update, you might be interested in the following issue:
#2820079: Convert \Drupal\book\Tests\BookTest::testBookOrdering() to a JavascriptTestBase test

Last couple of nitpicks then I think this is ready:

1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,120 @@
   + * @group session
   

   needs a different @group

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,120 @@
   +
   

   One newline too many.

3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,120 @@
   +      // Set the value for the role, triggering the machine name update.
   

   'role' Left over from when this used add role page.

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -0,0 +1,120 @@
   +      $generated_value = $machine_name_1_value->getHTML();
   ...
   +      $generated_value_2 = $machine_name_2_value->getHTML();
   

   These variables are not really needed, this can just be done in the assert right?

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -100,14 +103,18 @@
   +    $this->assertEquals(TRUE, $machine_name_1_wrapper->hasClass('visually-hidden'), 'The ID field must not be visible');
   

   Maybe add an short explanation why isVisible() doesn't work here.

6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
   @@ -100,14 +103,18 @@
   +    $last_test = array_pop($test_values);
   

   I would just use $test_values[1]['expected'], that way the $test_values can be reused in further test we might want to add later.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
@@ -0,0 +1,120 @@
+      // Get the generated machine name.
+      $generated_value = $machine_name_1_value->getHTML();
...
+      // Validate the generated machine name.
+      $this->assertEquals($test_info['expected'], $generated_value, $test_info['message']);
...
+      // Get value for the second machine name field.
+      $generated_value_2 = $machine_name_2_value->getHTML();
...
+      // Validate the second machine name field is empty.
+      $this->assertEmpty($generated_value_2, 'The second machine name field should still be empty');

WOW mate! I think you're the only one will understand me on another issue #2820079: Convert \Drupal\book\Tests\BookTest::testBookOrdering() to a JavascriptTestBase test. It's great code. Validate result on generated HTML ( ->getHTML() )

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/MachineNameTest.php
@@ -0,0 +1,120 @@
+      // Wait the set timeout for fetching the machine name.
+      $this->getSession()->wait(300);

It needed longer timeout and JS condition to aviod random failing. 300ms isn't always 300ms in JS :)

It needed longer timeout and JS condition to aviod random failing. 300ms isn't always 300ms in JS :)

Well waitOnAutocomplete also adds another wait(300), so the total wait before waiting for the ajax request to finish is already 600ms. I feel this should be sufficient. If that won't do then we need to not wait() a set number of ms, but just wait for the expected value to appear, but I think this is much cleaner.

@droplet I feel this is good to go now, but of course feel free to set back to needs work if you disagree on the timing thing!

@michielnugter nice work!

@Lendude,

OK. Let's go.

@michielnugter,

Thanks.

It will be stopped when the JS condition is met.

->wait(1000, JS-Condition)

@see: \vendor\behat\mink\src\Driver\DriverInterface.php

    /**
     * Waits some time or until JS condition turns true.
     *
     * @param int    $timeout   timeout in milliseconds
     * @param string $condition JS condition
     *
     * @return bool
     *
     * @throws UnsupportedDriverActionException When operation not supported by the driver
     * @throws DriverException                  When the operation cannot be done
     */
    public function wait($timeout, $condition);

In fact, the `waitOnAutocomplete()` should not set a fixed 300ms also. But it's out of this issue scope.

Actually we have a real example with similar problem over there, you can see how unstable the PhantomJS test is.
https://www.drupal.org/node/2782915#comment-11728915

Unless we think it's a UX issue and should be tested the action within 300ms execution time. Otherwise, we can increase the timeout there with JS condition.

All good.

Enhanced a little bit to cover 2nd machine name also. It's easy to use a bad selector & bubble up in JS

Committed and pushed 2cde5b6 to 8.3.x and d97733a to 8.2.x. Thanks!