Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
16.76 KB

See attached.

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeCreationTest.php
@@ -21,7 +23,7 @@ class NodeCreationTest extends NodeTestBase {
     // Enable dummy module that implements hook_node_insert for exceptions.
-    parent::setUp(array('node_test_exception', 'dblog'));
+    parent::setUp();

The comment should also be removed.

Otherwise looks very good.

ksenzee’s picture

I wonder if the comments should be moved to above the static property, instead of deleting them entirely. It might be useful in some cases to explain what the test modules are meant to do.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
17.02 KB

Sure.

tim.plunkett’s picture

Title: Convert node tests to use ::$modules instead of ::setUp($modules) » Convert node tests to use ::$modules property instead of parent::setUp($modules)

Keeping title in line.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Status: Reviewed & tested by the community » Needs work

Needs doxygen fix as discussed in IRC. :)

xjm’s picture

See the summary of #1711070: Convert tests to use ::$modules property instead of parent::setUp($modules) for how the docblock should be added. Also, note that the current docblock for NodeAccessRecordsTest should be switched so that it has a one-line summary (same as classes and methods do, though it doesn't need to start with a verb.) In this case we might get away with saying "Enable a module that implements node access API hooks and alter hooks". (Edit: I'm on the fence as to whether that should have the infamous s.) ;)

Edit: The fact that node_test implements the node access API is... interesting, to say the least, based on how it's used. But that's out of scope for this issue.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.09 KB

Rerolled for docblock fixes.

xjm’s picture

Status: Needs review » Needs work
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAccessRecordsTest.phpundefined
@@ -11,6 +11,14 @@ namespace Drupal\node\Tests;
+   * "Enable a module that implements node access API hooks and alter hook.

Stray quote. :) Also, "alter hooks" should probably be plural.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeCreationTest.phpundefined
@@ -11,6 +11,16 @@ use Drupal\Core\Database\Database;
+   * Modules to enable.
+   *
+   * Enable dummy module that implements hook_node_insert() for exceptions.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRSSContentTest.phpundefined
@@ -15,6 +15,16 @@ namespace Drupal\node\Tests;
+   * Modules to enable.
+   *
+   * Enable a module that implements hook_node_view().

We can probably remove the first lin here and use the second as the summary?

tim.plunkett’s picture

Status: Needs work » Closed (duplicate)

I agree, I'm making this change directly in the massive patch for #1711070: Convert tests to use ::$modules property instead of parent::setUp($modules)