Problem/Motivation

+++ b/core/modules/user/config/schema/user.views.schema.yml
@@ -56,10 +56,6 @@ views.argument_default.current_user:
-views.argument_default.node:
-  type: boolean
-  label: 'Content ID from URL'
-

Let's get a follow up to add a specific test for this then. Nice to have it fixed though.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 test-node-argument_default-plugin-2660446-14.patch9.47 KBtduong
#14 interdiff-2660446-11-14.txt1.8 KBtduong
#14 2660446-fake_test_only.patch10.06 KBtduong
#14 interdiff-fake_test_only.txt467 bytestduong
#11 test-node-argument_default-plugin-2660446-11.patch9.15 KBtduong
#11 interdiff-2660446-9-11.txt604 bytestduong
#9 test-node-argument_default-plugin-2660446-9.patch9.09 KBtduong
#9 interdiff-2660446-6-9.txt1.09 KBtduong
#6 test-node-argument_default-plugin-2660446-6.patch9.22 KBtduong
#6 interdiff-2660446-4-6.txt1.87 KBtduong
#4 test-node-argument_default-plugin-2660446-4.patch8.65 KBtduong

Comments

dawehner created an issue. See original summary.

tduong’s picture

tduong commented

I cannot find which issue is this follow up from ? Can you give some more details about the test please ?

edurenye’s picture

edurenye commented
Related issues: +#2633308: Views cache contexts are lost (and thus do not bubble) when rendering a view's block display.
tduong’s picture

tduong commented
Status: Active » Needs review
StatusFileSize
new test-node-argument_default-plugin-2660446-4.patch8.65 KB

Finally able to upload the test coverage for this default argument.

dawehner’s picture

dawehner
Primary language German
commented

Awesome!

  1. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
@@ -130,6 +137,36 @@ function testArgumentDefaultFixed() {
   /**
    * @todo Test node default argument.
    */

    Yeah, let's remove that todo! 

  2. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
@@ -130,6 +137,36 @@ function testArgumentDefaultFixed() {
+  function testArgumentDefaultNode() {

    Let's quickly make that a public function 

  3. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
@@ -130,6 +137,36 @@ function testArgumentDefaultFixed() {
+      'view all revisions',
...
+
+    // Confirm that the block is listed in the block administration UI.
+    $this->drupalGet('admin/structure/block/list/' . $this->config('system.theme')->get('default'));
+    $this->clickLinkPartialName('Place block');

    IMHO we can get rid of that bit of the testcode. Do you think it really adds test value?

tduong’s picture

tduong commented
Assigned: Unassigned » tduong
StatusFileSize
new interdiff-2660446-4-6.txt1.87 KB
new test-node-argument_default-plugin-2660446-6.patch9.22 KB

1) done
2) set public also for the other functions
3) most of other tests that place a block use these code lines so I took from them, but no, not really needed here. Dropped it :)

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Nice work!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
@@ -128,8 +135,34 @@ function testArgumentDefaultFixed() {
+    // Create a node where should show the node again in the block.

    Should this be:

    "Create a node where the view should be shown in the block"? 

  2. +++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
@@ -128,8 +135,34 @@ function testArgumentDefaultFixed() {
+    // Place the block, visit a page that displays the block, and check that
+    // the node we expect appears.
+    $this->drupalPlaceBlock("views_block:test_argument_default_node-block_1");
+    $this->drupalGet('node/' . $node->id());
+    $this->assertText($view->getTitle());

    This asserts for the view title, not the node title - is just the comment wrong?

tduong’s picture

tduong commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2660446-6-9.txt1.09 KB
new test-node-argument_default-plugin-2660446-9.patch9.09 KB

As I understand, the goal of this test is to check that using the contextual filter and selecting 'Content ID from URL' as default argument, then the node (title) is displayed on the node itself as a view block. Correct me if I'm wrong!

Fixed comment, fixed assertion for $node->getTitle().

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
@@ -162,7 +159,7 @@
-    $this->assertText($view->getTitle());
+    $this->assertText($node->getTitle());

What is this actually checking? I mean the node title of the node itself probably appears alright on the page.

tduong’s picture

tduong commented
StatusFileSize
new interdiff-2660446-9-11.txt604 bytes
new test-node-argument_default-plugin-2660446-11.patch9.15 KB

Replaced that assertion with an assertFieldByXpath. Sorry, I should have thought that...

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/src/Tests/Plugin/ArgumentDefaultTest.php
@@ -128,8 +135,32 @@ function testArgumentDefaultFixed() {
+    $this->drupalGet('node/' . $node->id());
+    $xpath = '//div/div/div/div/div/span/a';

Is there no class or something like that we can leverage? This is super unspecified selector.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Can you upload a fake "test-only" patch that breaks that plugin? In the argument default plugin, instead of returning a value, just do a return NULL; or so and then the test should fail. upload that as a patch.

tduong’s picture

tduong commented
StatusFileSize
new interdiff-fake_test_only.txt467 bytes
new 2660446-fake_test_only.patch10.06 KB
new interdiff-2660446-11-14.txt1.8 KB
new test-node-argument_default-plugin-2660446-14.patch9.47 KB

Discussed with @berdir and it was pretty challenging to make this test to fail...

Anyway, changed the way we check for the node title's existence IN the block displayed in the pages by looking for the position of the node title in the block XML as a string.

On the fake_test_only, since we always cache the 'url' argument

\Drupal\views\Plugin\views\argument\ArgumentPluginBase::getCacheContexts()
// By definition arguments depends on the URL.

there is no other way to break the code to make a test fail but forcing a 'broken' return in \Drupal\node\Plugin\views\argument_default\Node::getArgument().
Hope this makes sense and is good enough. :D

The last submitted patch, 14: 2660446-fake_test_only.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Nice, I like that!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Yeah I like that too.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 2a27ee8 on 8.1.x 
    Issue #2660446 by tduong: Test the node argument_default plugin

  • catch committed 29437c4 on 8.0.x 
    Issue #2660446 by tduong: Test the node argument_default plugin

(cherry...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.