Convert CommentActionsTest to kernel test

This patch needs some work and probably needs a re-roll. The details for how to do so can be found in the parent issue linked in the issue summary.

I will write a summary for this issue. See https://www.drupal.org/contributor-tasks/write-issue-summary.

Working on this during the Dublin sprints

Working on this with Eyal Shalev

https://gist.github.com/anonymous/562aae1d43fff43ba59f7c78c0639070

We fixed the naming issue, and we provided a name for the test user. Here is a new patch.

@johanv and @Eyal Shalev worked on this patch together.

.

We (wizonesolutions, Eyal Shalev and me) split up the test into 5 tests. We tested it locally, and it still works.

1. +++ b/core/modules/comment/src/Tests/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   + * @file
   

   @file isn't required.

2. +++ b/core/modules/comment/src/Tests/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +    // Create format without filters to prevent filtering
   

   Missing full stop.

3. +++ b/core/modules/comment/src/Tests/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +   * @param array $keywords
   

   Missing parameter comment.

4. +++ b/core/modules/comment/src/Tests/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +   * @return \Drupal\Core\Action\ActionInterface
   

   Description for return value is missing and separate @param and @return sections by a blank line.

5. +++ b/core/modules/comment/src/Tests/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +    return $action_stored;
   

   Check the type of @return value

6. +++ b/core/modules/comment/src/Tests/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +  }
   

   An empty line before class end.

Tests ran successfully on local environment.

Here is the revised patch. No logic changes, so if the tests in #16 pass, and this is otherwise good, I think we should RTBC this :)

You uploaded an empty patch :-)

I have to leave now. I reviewed the latest patch, it handles the comments of mmrares. So if the test bot does not find any problems, I think this is RTBC.

Fixed file comment.

I confirm that issues from #17 are resolved in the patch #25
There are no logic changes and test in #16 passed so this will still pass

@wizonesolutions, @johanv. One thing that helps when creating new patches is to create an interdiff so that we can see the specific changes between patches. See https://www.drupal.org/documentation/git/interdiff

Fixing inconsistent whitespace and ensuring test method docblocks start with a third-person verb per IRC comment by @YesCT.

No code changes, so tests will still pass.

@condor616 reviewed this live as I wrote it.

Just a nit here, not enough to un-rtbc.

+++ b/core/modules/comment/src/Tests/Action/CommentActionsKernelTest.php
@@ -0,0 +1,193 @@
+    \Drupal::currentUser()->setAccount($account);
...
+    \Drupal::service('router.builder')->rebuild();

All usages of \Drupal should be $this->container.

Applying the changes suggested by Mile23

+++ b/core/modules/comment/src/Tests/Action/CommentActionsKernelTest.php
@@ -74,7 +74,7 @@
-    $this->container->get('current_user')->setAccount($account);
+    \Drupal::currentUser()->setAccount($account);

@@ -83,7 +83,7 @@
-    $this->container->get('router.builder')->rebuild();
+    \Drupal::service('router.builder')->rebuild();

The interdiff is the reverse. Interdiffs are created in format:

interdiff <previous patch> <next patch>.

I reviewed the patch manually and \Drupal:: is indeed removed in favor of $this->container->get.

Submit a new patch to be tested against 8.4.x and with a fixed interdiff.

The last submitted patch, 35: 2742997-35.patch, pass testing.

The new test looks good and it seems to be in the correct namespace.

It's a kernel test not a unit test.

1. +++ b/core/modules/comment/tests/src/Kernel/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +    $action = Action::load('comment_save_action');
   +    $action->execute([$this->comment]);
   +    $this->assertFalse($this->comment->isNew(), 'Comment saved');
   

   This should assert the comment is new before executing the action.

2. +++ b/core/modules/comment/tests/src/Kernel/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +    $this->comment->save();
   +    // Test comment_unpublish_by_keyword_action for comment body.
   +    $action_stored = $this->createUnpublishByKeywordAction([$this->keyword2]);
   +    $action_stored->execute([$this->comment]);
   +    $this->assertFalse($this->comment->isPublished(), 'Comment unpublished by body keyword');
   

   This should assert the comment is published after saving.

3. +++ b/core/modules/comment/tests/src/Kernel/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +    $this->comment->save();
   +
   +    // Test comment_publish_action.
   +    $action = Action::load('comment_publish_action');
   +    $action->execute([$this->comment]);
   +    $this->assertTrue($this->comment->isPublished(), 'Comment published');
   

   This needs to assert the comment is not published after saving.

4. +++ b/core/modules/comment/tests/src/Kernel/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +    $this->comment->save();
   +
   +    // Test comment_unpublish_action.
   +    $action = Action::load('comment_unpublish_action');
   +    $action->execute([$this->comment]);
   +    $this->assertFalse($this->comment->isPublished(), 'Comment unpublished');
   

   This needs to assert that the comment is published after saving.

5. +++ b/core/modules/comment/tests/src/Kernel/Action/CommentActionsKernelTest.php
   @@ -0,0 +1,189 @@
   +    $this->comment->save();
   +    $action_stored = $this->createUnpublishByKeywordAction([$this->keyword2]);
   +    // Test comment_unpublish_by_keyword_action for comment subject.
   +    $action_stored
   +      ->set('configuration', ['keywords' => [$this->keyword1]])
   +      ->save();
   +    $this->comment->setPublished(TRUE);
   +    $action_stored->execute([$this->comment]);
   +    $this->assertFalse($this->comment->isPublished(), 'Comment unpublished by subject');
   

   This needs to test that the comment is published before triggering the action. Also I'd expect a test to ensure the is the comment doesn't contain the keyword this action doesn't unpublish the comment. Plus what is the difference between testCommentUnPublishByKeywordAction and testUnpublishByKeywordAction?

Reroll patch from #38 for 8.7.x
Still, need to add all notices from #43

Reroll patch from #38 for 8.7.x
Still, need to add all notices from #43

Adding proposal in #43 to patch.

This test needs clean-up because save/delete/publish/unpublish actions already tested because they are now core ones.
Moreover module's actions are deprecated except \Drupal\comment\Plugin\Action\UnpublishByKeywordComment one

Also there's already functional test which better to convert to kernel one based on patch here

Converted, changed test to test the only action and all ones shipped by default configured
The functional test us removed

Actions to save/delete use own tests, the same as [un]publish are using only kernel tests

1. +++ b/core/modules/comment/tests/src/Kernel/CommentActionsTest.php
   @@ -0,0 +1,159 @@
   +    $account = User::create([
   +      'name' => 'test user',
   +    ]);
   +    $account->save();
   +    $this->container->get('current_user')->setAccount($account);
   

   We have a method on UserCreationTrait for this now

2. +++ b/core/modules/comment/tests/src/Kernel/CommentActionsTest.php
   @@ -0,0 +1,159 @@
   +    $this->keywords = [$this->randomMachineName(), $this->randomMachineName()];
   

   any reason to make this a class property? can't we just use inline variables in the second method?

Fixed #56.1 using trait from parent class

About #56.2 test needs this keywords in setup (create comment with keywords) and test method

Patch could not be applied cleanly to 8.9.x-dev.

Rerolled patch 2742997-58.patch.

It was not an auto merge. See diff of two most recent patches for details: reroll_diff_58-61.txt.

Rerolled

The patch looks good. Just one minor request:

+++ b/core/modules/comment/tests/src/Kernel/CommentActionsTest.php
@@ -0,0 +1,153 @@
+    $action = Action::load('comment_delete_action');
+    $action->execute([$this->comment]);

Can we add an assertion that This->comment has been deleted.

There's no actual delete happens, that's why previous test missing this hunk (already covered by comment-admin list-builder and views test)

So just make sure the valid values are stored in tempstore is enough

+++ b/core/modules/comment/tests/src/Kernel/CommentActionsTest.php
@@ -0,0 +1,153 @@
+    $action = Action::load('comment_delete_action');
+    $action->execute([$this->comment]);
+    /** @var \Drupal\Core\TempStore\PrivateTempStoreFactory $temp_store */
+    $temp_store = $this->container->get('tempstore.private');
+    $account_id = $this->container->get('current_user')->id();
+    $store_entries = $temp_store->get('entity_delete_multiple_confirm')->get($account_id . ':comment');
+    $this->assertSame([$account_id => ['en' => 'en']], $store_entries);

That's what the action is doing, it prepares tempstore with ID's to delete.
The hunk specifically added to cover case of delete action.

See \Drupal\Core\Action\Plugin\Action\Derivative\EntityDeleteActionDeriver::getDerivativeDefinitions()

Btw there's only \Drupal\comment\Plugin\Action\UnpublishByKeywordComment action left in comment module, so delete test could be removed

@andypost: Thank you for the explanation.

All changes now look good to me.
For me it is RTBC.

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!