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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

tduong’s picture

tduong CreditAttribution: tduong at MD Systems GmbH 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 CreditAttribution: edurenye at MD Systems GmbH 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 CreditAttribution: tduong at MD Systems GmbH commented
Status: Active » Needs review
FileSize
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
CreditAttribution: dawehner as a volunteer and at Chapter Three 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 CreditAttribution: tduong at MD Systems GmbH commented
Assigned: Unassigned » tduong
FileSize
interdiff-2660446-4-6.txt1.87 KB
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
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Nice work!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove 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 CreditAttribution: tduong at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
interdiff-2660446-6-9.txt1.09 KB
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
CreditAttribution: dawehner as a volunteer and at Chapter Three 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 CreditAttribution: tduong at MD Systems GmbH commented
FileSize
interdiff-2660446-9-11.txt604 bytes
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
CreditAttribution: dawehner as a volunteer and at Chapter Three 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
CreditAttribution: Berdir at MD Systems GmbH 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 CreditAttribution: tduong at MD Systems GmbH commented
FileSize
interdiff-fake_test_only.txt467 bytes
2660446-fake_test_only.patch10.06 KB
interdiff-2660446-11-14.txt1.8 KB
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
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Nice, I like that!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove 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.