Improve core.api.php documentation about browsertests and javascript tests

I will read this later in detail, probably... nitpick: the correct capitalization: JavaScript

Hi, back from vacation...

This is a pretty good start. I took a look at the text and have some proofreading suggestions... if you want to do some rewriting I think that would be a good idea. In a few days when I get done with more of the mounds of other tasks that have accumulated in my 3 weeks off, I may have time to rewrite also...

Meanwhile, here is a nitpicky review:

1. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * Functional tests are written using PHPUnit as underlying framework, we call
   + * them BrowserTestBase, as they use a simulated browser, which can click links
   + * goto URLs, post to forms, and much more. To write a functional test:
   

   Rewrite of this first sentence:

   Functional tests are written using the BrowserTestBase base class, and use PHPUnit as their underlying framework. They use a simulated browser, in which the test can click links, visit URLs, post to forms, etc.

2. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * - You need to extend \Drupal\Tests\BrowserTestBase. There are a couple of
   

   Take out "You need to"

3. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + *   base assertions for stuff related with pure data ($this->assertEquals), but
   + *   more important with $this->assertSession() you get a
   + *   \Drupal\Tests\WebAssert object containing browser related assertions like
   + *   linkExists().
   

   I'm not sure what this means and how it relates to extending BrowserTestBase?

4. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * - The expected folder is yourmodule/tests/src/Functional and using a
   + *   namespace like Drupal\Tests\yourmodule\Functional.
   + *   a @group annotation, which gives information about the test.
   

   This is a bit garbled... and should it be two bullet points (one about the folder and namespace, and one about the @group)?

   Also use the word "directory" not "folder" as a general rule in Drupal docs

5. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * - You may also override the default setUp() method, which can set be used to
   + *   set up content types and similar procedures.
   

   ==> which can be used
   [take out word "set" near end of line]

6. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * provide functional test coverage for those Drupal provides another base
   

   ==>
   provide functional test coverage for this, Drupal provides...

7. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * class, \Drupal\FunctionalJavaScriptTests\JavascriptTestBase, which works
   

   Is this the correct namespace?

8. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * really similar to BrowserTestBase, exists.
   

   The end of this sentence is garbled

9. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * - When clicking a link/button with ajax behaviour, you need to keep in mind
   

   ajax => Ajax

@jhodgdon: took all your input and updated the docs with that. Also carried some of the info to https://www.drupal.org/node/2716803. There is an interdiff, but not much clearer than just reading the new patch ;)

1. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * @section write_functional_phpunit Write functional tests (phpunit)
   ...
   + * @section write_jsfunctional_phpunit Write JavaScript tests (phpunit)
   

   s/phpunit/PHPUnit/

   functional PHP
   functional JavaScript

2. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * - You may also override the default setUp() method, which can be used to set
   + *   up content types and similar procedures.
   

   Should this say "But don't forget to call the parent method."?

3. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + *   put such modules under the yourmodule/tests/modules directory.
   

   s/under/in/

4. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * - To run JavaScript tests, set up phantomjs, see core/tests/README.md.
   

   s/phantomjs/PhantomJS/

5. +++ b/core/core.api.php
   @@ -1121,6 +1121,42 @@
   + * - When clicking a link/button with Ajax behaviour attached, you need to keep
   

   s/behaviour/behavior/ :)

Updated for everything but 3.

I contest 3, because the modules would not be directly put *in* that directory I think but rather in directories *under* that directory, no?

#8: I think a native speaker should look at this — I honestly don't know :) I'm just used to reading in directory, not under directory.

I agree with Gabor -- I would say "under", because ... well... actually I think I would say that each test module would go in its own subdirectory of yourmodule/tests/modules.

Some other thoughts:

1. This is looking pretty good!!!

2. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + *   up content types and similar procedures. Don't forget to invoke the parent
   

   invoke => call

3. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * JavascriptTestBase class, which works really similar to BrowserTestBase:
   

   grammar: "works really similar" is not really quite right.

   I think I would actually rewrite this whole sentence to only say:

   To write a functional test that relies on JavaScript:

   (because the first bullet point already says to use this JavascriptTestBase class)

4. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * - Place the test into yourmodule/tests/src/FunctionalJavascript and use
   

   To be similar to the above section, this should say:

   Place the test in the yourmodule/tests/src/FunctionalJavascript directory ...

5. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + *   the Drupal\Tests\yourmodule\FunctionalJavascript namespace
   

   end in .

6. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * - To run JavaScript tests, set up PhantomJS, see core/tests/README.md.
   

   ; not , before see.

   Also... the readme.md file doesn't actually explain how to *set up* PhantomJS. It only includes a command for starting it. So maybe give a link to more information about PhantomJS?

Great comments :) I was not sure setting up phantomjs is just about downloading it, but https://www.drupal.org/node/2716803 also just links there, so I think we can do that too. Fixed all the concerns raised.

The documentation reads very well now, IMO.

I cannot really review it for accuracy though... so I hesitate to mark it RTBC.

Looks good!

Minor things that should not hold up this patch:

1. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * @section write_jsfunctional_phpunit Write functional JavaScript tests (phpunit)
   

   Over 80 characters, but this probably has to be on one line. I would just call it write_functional_js.

   Should we correctly capitalize PHPUnit everywhere?

2. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * - When clicking a link/button with Ajax behavior attached, you need to keep
   + *   in mind that the underlying browser might need a while to deliver changes
   + *   to the HTML. Use $this->assertSession()->assertWaitOnAjaxRequest() to wait
   + *   for that.
   

   The important part here is that you never wait blindly a certain amount of milli seconds but always have a JS condition attached to it. assertWaitOnAjaxRequest() already does that for you, but if you want to test a non-Ajax user interaction on the page you will use $this->getSession()->wait(). Always use a JS condition as the second parameter there, for example to check if something is visible after triggering the JS interaction.

   But maybe that is too detailed for this section here? Should not hold up this patch.

This is a huge improvement!

1. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * Functional tests are written using the BrowserTestBase base class, and use
   

   We can replace the words "are written using" here with "extend".

2. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + *   the Drupal\Tests\yourmodule\Functional namespace.
   ...
   + *   and use the Drupal\Tests\yourmodule\FunctionalJavascript namespace.
   

   Missing leading slash here.

3. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * - Methods in your test class whose names start with 'test', and which have
   + *   no arguments are the actual test cases. Each one should test a logical
   

   I think the comma here is incorrect and makes the sentence confusing. To make it clearer, let's flip the sentence around:

   Add test cases by adding method names that start with 'test' and have no arguments, for example testYourTestCase().

4. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + *   subset of the functionality, and each one runs in a new, isolated test
   + *   environment, so it can only rely on the setUp() method, not what has
   + *   been set up by other test methods.
   

   This sentence is a bit of a run-on. Let's start a new sentence with "Each one runs in a new..." and instead say "Each method runs in a new..."

5. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * A lot of functionality relies on JavaScript. To write a functional test that
   

   Do we really need to tell people that "A lot of functionality relies on JavaScript"?

6. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + * - When clicking a link/button with Ajax behavior attached, you need to keep
   

   We can remove the words "you need to" here.

7. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + *   in mind that the underlying browser might need a while to deliver changes
   

   I'd suggest "might take time" instead of "might need a while".

8. +++ b/core/core.api.php
   @@ -1121,6 +1121,43 @@
   + *   for that.
   

   Instead of "for that" maybe "for the Ajax request to finish"?

9. Should we add a link to the longer handbook docs as well?

(Fixing issue credit.)

#17 addressed everything in #14, except point 9. It also did a little bit extra. One mistake was made though:

+++ b/core/core.api.php
@@ -1122,13 +1123,12 @@
+ *   the Drupal\Tests\yourmodule\Functional\ namespace.

@@ -1137,26 +1137,26 @@
+ *   directory and use the Drupal\Tests\yourmodule\FunctionalJavascript/

This now has a trailing slash, but it needed a trailing slash :D

Is there a full docs page other than https://www.drupal.org/node/2716803 and https://www.drupal.org/node/2469723 (both change notices)?

Doc pages:
https://www.drupal.org/docs/8/phpunit
https://www.drupal.org/docs/8/phpunit/running-phpunit-tests
https://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial
Currently missing is a JavascriptTestBase setup + tutorial.

Improving the last bits of this issue is a novice task.

Took this because I think solving this soon is important for the understandability of this system.

Fixed the slash found by @wimleers.

Added a link to the PHPUnit tutorial. The parent page is already linked in the PHPUnit section so no point in linking it here (again).

Anything else?

Thanks, looks good!

1. +++ b/core/core.api.php
   @@ -1121,6 +1122,45 @@
   + *   the Drupal\Tests\yourmodule\Functional\ namespace.
   ...
   + *   directory and use the Drupal\Tests\yourmodule\FunctionalJavascript\
   

   These are still missing the leading slash, and still have a trailing slash that they should not.

2. +++ b/core/core.api.php
   @@ -1121,6 +1122,45 @@
   + * For more details, see:
   + * - https://www.drupal.org/docs/8/phpunit/phpunit-browser-test-tutorial for
   + *   a full tutorial on how to write functional PHPUnit tests for Drupal.
   

   The formatting here is weird (why a single-element list?) and we should use @link/@endlink instead. I guess it is being copied from elsewhere in the class?

I guess point 2 is repeating a pattern from elsewhere but point 1 (which is also #14.2) is still not addressed.

Thanks!

Also, I do think it is worth repeating links from the previous section, since we should not assume people have read the entire thing if they only want to write a browser test; and also referencing handbook resources for writing and running the JSTB tests (even if we just add a stub for now that links the CR and alexpott's blog post or whatever).

Good idea @dawehner.

@link/@endlink should only be used if you want to have link text. Otherwise, just putting the URL in alone is good. Agreed that a one-item list is silly though! :)

All right, I created a page at https://www.drupal.org/docs/8/phpunit/phpunit-javascript-testing-tutorial where I copied the change notice from https://www.drupal.org/node/2716803. I personally believe that an inviting empty stub is more harmful than not having anything there at all. This is at least marked incomplete so people MAY help update it. Still don't think creating that page was novice and it was required to fix @xjm's concerns.

- Updating the namespaces as suggested (there are several other namespaces documented in this file without a leading slash but those are out of scope for the patch).
- Repeated the phpunit general docs link as suggested by @xjm.
- Added a link to the newly existing JS testing doc page.

Nice.

Committed and pushed ea1bc99 to 8.3.x and 8ab9f5c to 8.2.x. Thanks!