From #764558: Remove Trigger module from core.

A good bit of test coverage for actions was implicit in trigger.module tests. With that removed, it would be good to add some additional test coverage for actions.

From sun's comment on #1797658: Add test coverage for Actions module:

There are two main aspects to test:

1) Low-level API of executing actions (i.e., actions_do()).

2) Configuration/UI for actions.

Ideally also 3) Testing of individual actions (hopefully soon plugins), but those should rather be unit tests, detached from the actual action API.

2) should be relatively straightforward. Just go through the entire available UI and verify all possible operations.

1), however, is pretty advanced. Especially, because there is no way at all in core to trigger the operation. This likely means that it should be a unit test as well, since all we can reasonably do is to verify that certain input leads to certain output/actions.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

http://drupal.org/project/code_coverage would be really useful here to see how much coverage we actually have from the existing actions tests as well as how much we'll lose along with the trigger tests.

wadmiraal’s picture

Adding tests for the node module. Coming up.

wadmiraal’s picture

FileSize
4.04 KB

Here are some tests covering the Node actions.

Edit: Ignore this one, see patch in #7

wadmiraal’s picture

FileSize
1.18 KB

Here are some tests covering the User actions.

Sidenote: is it ok to post these separately ? Should I bundle them ?

Edit: Ignore this one, see patch in #7

wadmiraal’s picture

Question, before I go on: do these tests need to cover the actual triggering functionality ? Or is calling actions_do() just fine (as the triggering is not actually part of the actions and should be tested separately, right ?).

wadmiraal’s picture

Status: Active » Needs review
FileSize
1.16 KB

User test case did not cover passing an Entity as a parameter (instead of a User). Updated the test.

wadmiraal’s picture

Ok, terribly sorry. Got the comments in the php files wrong.

dags’s picture

dags’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Actions/UserTest.phpundefined
@@ -0,0 +1,50 @@
+ * Definition of Drupal\system\Tests\Actions\UserTest.

standards have changed a little. we are using
Contains instead of "Definition of". Also a \ before Drupal\whatever
http://drupal.org/node/1354

+++ b/core/modules/system/lib/Drupal/system/Tests/Actions/UserTest.phpundefined
@@ -0,0 +1,50 @@
+  ¶

there is trailing whitespace in a bunch of places.

Installing dreditor and reviewing with that makes it easy to spot that. dreditor highly recommended.

+++ b/core/modules/system/lib/Drupal/system/Tests/Actions/UserTest.phpundefined
@@ -0,0 +1,50 @@
+    // Trigger action on a user entity

comments need to be sentences that end in a period (this is in a few places).

http://drupal.org/node/1354#drupal

dags’s picture

Status: Needs work » Needs review
FileSize
6.65 KB
5.25 KB

I had some spare time today so I decided to reroll this. I combined the two patches in #7 into 1 patch file, making the changes YesCT pointed out in #10, as well as some other things. The testNodePropertiesActions() function now uses the same $node object throughout - we shouldn't need to create a new $node for each assertion, right?

Status: Needs review » Needs work

The last submitted patch, node-user-action-tests-1412964-11.patch, failed testing.

star-szr’s picture

babruix’s picture

Status: Needs work » Needs review
FileSize
6.4 KB

Problem was that by default action module is turned off.

Status: Needs review » Needs work

The last submitted patch, system-node-user-action-tests-1412964-14.patch, failed testing.

babruix’s picture

Hm, \Drupal\Core\Extension\ModuleHandlerInterface::isLoaded() returns false - that why exception was thrown:

Exception: theme() may not be called until all modules are loaded. in theme() (line 968 of /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/theme.inc).
theme('node', Array)
drupal_render(Array)
node_unpublish_by_keyword_action(Object, Array, NULL, NULL)
actions_do('node_unpublish_by_keyword_action', Object, Array)
babruix’s picture

Status: Needs work » Needs review
FileSize
6.57 KB

Added drupal_container()->get('module_handler')->loadAll(); before calling actions_do('node_unpublish_by_keyword_action') .

YesCT’s picture

@babruix an interdiff is awesome when adding a new patch to an issue:
For instructions on creating an interdiff, see http://drupal.org/node/1488712 Or, http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...

babruix’s picture

FileSize
1.25 KB

Here is interdiff beween last two patches.

xjm’s picture

Status: Needs review » Closed (duplicate)

This is no longer needed following #1846172: Replace the actions API.

willieseabrook’s picture

Status: Closed (duplicate) » Needs review

#1846172: Replace the actions API didn't come with test coverage. #17 has more test coverage

willieseabrook’s picture

See comment 29 on #874624: Optimization of *_unpublish_by_keyword_action where I have created a new file core/modules/node/lib/Drupal/node/Tests/NodeActionsTest.php

willieseabrook’s picture

Issue summary: View changes

add comment from duplicate issue

penyaskito’s picture

Issue summary: View changes

What is the next step here? Should we be closed?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Closed (duplicate)
+++ b/core/modules/system/lib/Drupal/system/Tests/Actions/NodeTest.php
@@ -0,0 +1,128 @@
+class NodeTest extends WebTestBase {

Most of the logic in here could be done in a kernel test (\Drupal\KernelTests\KernelTestBase).

However, most of the functions being tested have since been removed in #1846172: Replace the actions API, which added tests, so I'm marking this as a duplicate of that. Please re-open with an issue summary update if there is still work to be done.