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.
Comment | File | Size | Author |
---|---|---|---|
#19 | interdiff.txt | 1.25 KB | babruix |
#17 | system-node-user-action-tests-1412964-17.patch | 6.57 KB | babruix |
#14 | system-node-user-action-tests-1412964-14.patch | 6.4 KB | babruix |
#11 | node-user-action-tests-1412964-11.patch | 5.25 KB | dags |
#11 | interdiff.txt | 6.65 KB | dags |
Comments
Comment #1
catchhttp://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.
Comment #2
wadmiraal CreditAttribution: wadmiraal commentedAdding tests for the node module. Coming up.
Comment #3
wadmiraal CreditAttribution: wadmiraal commentedHere are some tests covering the Node actions.
Edit: Ignore this one, see patch in #7
Comment #4
wadmiraal CreditAttribution: wadmiraal commentedHere 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
Comment #5
wadmiraal CreditAttribution: wadmiraal commentedQuestion, 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 ?).Comment #6
wadmiraal CreditAttribution: wadmiraal commentedUser test case did not cover passing an Entity as a parameter (instead of a User). Updated the test.
Comment #7
wadmiraal CreditAttribution: wadmiraal commentedOk, terribly sorry. Got the comments in the php files wrong.
Comment #8
dags CreditAttribution: dags commentedClosed duplicate issue #1797658: Add test coverage for Actions module.
Comment #8.0
dags CreditAttribution: dags commentedUpdated issue summary.
Comment #9
star-szrClosed #265940: TestingParty08: tests for comment triggers/actions as a duplicate as well.
Comment #10
YesCT CreditAttribution: YesCT commentedstandards have changed a little. we are using
Contains instead of "Definition of". Also a \ before Drupal\whatever
http://drupal.org/node/1354
there is trailing whitespace in a bunch of places.
Installing dreditor and reviewing with that makes it easy to spot that. dreditor highly recommended.
comments need to be sentences that end in a period (this is in a few places).
http://drupal.org/node/1354#drupal
Comment #11
dags CreditAttribution: dags commentedI 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?
Comment #13
star-szrRelated: #244093: Node and comment actions are (still) completely broken and have broken tests too
Comment #14
babruix CreditAttribution: babruix commentedProblem was that by default action module is turned off.
Comment #16
babruix CreditAttribution: babruix commentedHm, \Drupal\Core\Extension\ModuleHandlerInterface::isLoaded() returns false - that why exception was thrown:
Comment #17
babruix CreditAttribution: babruix commentedAdded
drupal_container()->get('module_handler')->loadAll();
before calling actions_do('node_unpublish_by_keyword_action') .Comment #18
YesCT CreditAttribution: YesCT commented@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-...
Comment #19
babruix CreditAttribution: babruix commentedHere is interdiff beween last two patches.
Comment #20
xjmThis is no longer needed following #1846172: Replace the actions API.
Comment #21
willieseabrook CreditAttribution: willieseabrook commented#1846172: Replace the actions API didn't come with test coverage. #17 has more test coverage
Comment #22
willieseabrook CreditAttribution: willieseabrook commentedSee 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
Comment #22.0
willieseabrook CreditAttribution: willieseabrook commentedadd comment from duplicate issue
Comment #23
penyaskitoWhat is the next step here? Should we be closed?
Comment #25
jhedstromMost 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.