Comments

dawehner created an issue. See original summary.

dawehner’s picture

Status: Active » Needs review
FileSize
2.73 KB

Here is a quick start

jhodgdon’s picture

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

dawehner’s picture

Just some small nitpicks

jhodgdon’s picture

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
4.29 KB

@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 ;)

Wim Leers’s picture

  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/ :)

Gábor Hojtsy’s picture

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?

Wim Leers’s picture

#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.

jhodgdon’s picture

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?

Gábor Hojtsy’s picture

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.

jhodgdon’s picture

The documentation reads very well now, IMO.

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

klausi’s picture

Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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?
xjm’s picture

(Fixing issue credit.)

dawehner’s picture

Working on it.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
5.05 KB
Wim Leers’s picture

Status: Needs review » Needs work

#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

Gábor Hojtsy’s picture

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

klausi’s picture

klausi’s picture

Issue tags: +Novice

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
4.39 KB
1.07 KB

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?

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good!

xjm’s picture

Status: Reviewed & tested by the community » Needs work
  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!

xjm’s picture

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).

dawehner’s picture

Issue tags: +Novice

Sure, let's get someone to improve it.

xjm’s picture

Good idea @dawehner.

jhodgdon’s picture

@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! :)

Gábor Hojtsy’s picture

Issue tags: -Novice

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.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.79 KB
2.16 KB

- 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.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @Gábor Hojtsy for the improvements! I strongly believe getting the information out rather sooner than later is helpful. We can and should improve continuously.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice.

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

  • alexpott committed ea1bc99 on 8.3.x
    Issue #2810621 by Gábor Hojtsy, dawehner, xjm, jhodgdon, klausi, Wim...

  • alexpott committed 8ab9f5c on 8.2.x
    Issue #2810621 by Gábor Hojtsy, dawehner, xjm, jhodgdon, klausi, Wim...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.