Better explain the $modules variable in kernel tests [#2709581]

Comment	File	Size	Author
#16	interdiff-2709581-7-16.txt	677 bytes	pepegarciag
#16	better_explain_the-2709581-16.patch	925 bytes	pepegarciag
#7	2709581-v2.patch	929 bytes	Torenware
#2	2709581-v1.patch	888 bytes	Torenware

Comment #1

19 April 2016 at 21:49

Torenware created an issue. See original summary.

Log in or register to post comments

Status	File	Size
new	2709581-v1.patch	888 bytes

Comment #3

Torenware commented 19 April 2016 at 22:00

Log in or register to post comments

Comment #4

dawehner

German

commented 19 April 2016 at 22:48

Sure why not. Can we add the same documentation to BrowserTestBase as well?

Log in or register to post comments

Comment #5

Torenware commented 19 April 2016 at 23:10

Yeah, I'd be willing to do that. Any comments on the wording as changed?

Log in or register to post comments

Comment #6

jhodgdon

she/her

English

commented 20 April 2016 at 13:37

Status:

Needs review

» Needs work

Thanks for making this issue and patch! Seems like a good idea to clarify it, in all of the base classes that have this member variable.

Wording... hm...

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -139,7 +139,13 @@
    * Test classes extending this class, and any classes in the hierarchy up to
    * this class, may specify individual lists of modules to enable by setting
    * this property. The values of all properties in all classes in the class
-   * hierarchy are merged.
+   * hierarchy are merged. So you do not need to re-declare modules that are
+   * declared in superclasses of your test class.
+   *
+   * An example:
+   * @code
+   * public static $modules = ['my_extra_module'];
+   * @endcode

I think rather than adding extra wording to what is there and adding a code sample, we should instead fix the unclear wording that was there.

I suggest replacing the entire paragraph that starts with "Test classes..." with the following, or something like this:

The test runner will merge the $modules lists from this class, the class it extends, and so on up the class hierarchy. So, it is not necessary to include modules in your list that a parent class has already declared.

And I don't really think we need an @code example?

Log in or register to post comments

Comment #7

Torenware commented 21 April 2016 at 04:55

Status:

Needs work

» Needs review

Status	File	Size
new	2709581-v2.patch	929 bytes

@jhodgdon -- I've changed the language accordingly.

@dawehner -- Funny thing: there's no public static $modules declared in BrowserTestBase. So there's no comment to fix. But... looking at the code, I see the following:

    // Collect modules to install.
    $class = get_class($this);
    $modules = array();
    while ($class) {
      if (property_exists($class, 'modules')) {
        $modules = array_merge($modules, $class::$modules);
      }
      $class = get_parent_class($class);
    }

So my sense is that there really ought to be a $modules class variable declared in BrowserTestBase, even if there isn't one now. And it should probably have a similar comment to the one used in KernelTestBase.

That said, it probably needs a slightly different comment than KernelTestBase. So what do you want me to do here?

Log in or register to post comments

Comment #8

dawehner

German

commented 21 April 2016 at 10:16

@dawehner -- Funny thing: there's no public static $modules declared in BrowserTestBase. So there's no comment to fix. But... looking at the code, I see the following:

Oh yeah, well then let's add the same kind of documentation.

Log in or register to post comments

Comment #9

jhodgdon

she/her

English

commented 21 April 2016 at 14:56

Status:

Needs review

» Needs work

Agreed, we should add the $modules variable, and docs, to BrowserTestBase. I just checked and it is not inheriting $modules from any base class. I don't see why it would need a different comment from the one in KernelTestBase?

Setting to Needs Work for that.

Log in or register to post comments

Comment #10

dawehner

German

commented 21 April 2016 at 15:14

I don't see why it would need a different comment from the one in KernelTestBase?

Me neither, cool

Log in or register to post comments

Comment #11

dawehner

German

commented 21 April 2016 at 15:29

I totally agree that the new comment makes it easier to understand.

Log in or register to post comments

Comment #12

joachim commented 21 April 2016 at 15:37

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -136,10 +136,9 @@
+   * it extends, and so on up the class hierarchy. So, it is not necessary to

The comma after 'So' isn't needed. In fact, the 'So' isn't either.