Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
13.29 KB

One test breaks at the moment, but the rest are working fine.

Status: Needs review » Needs work

The last submitted patch, drupal-1934558-1.patch, failed testing.

dawehner’s picture

Title: Create a test for views data handlers. » Create a test for views date handlers.

yeah data, date it's all the same.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -VDC

#1: drupal-1934558-1.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, drupal-1934558-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.41 KB
1.23 KB

New version, which might not conflict with the testbot? (no idea why this is needed).

Status: Needs review » Needs work

The last submitted patch, drupal-1934558-6.patch, failed testing.

olli’s picture

Status: Needs work » Needs review

This looks great.

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/ArgumentDateTest.php
@@ -0,0 +1,325 @@
+    $this->installSchema('user', 'role_permission');

I think this can be removed too once the handlers are moved to views.

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/ArgumentDateTest.php
@@ -0,0 +1,325 @@
+    $view->setDisplay('embed_3', array(1));
+    $this->executeView($view);

executeView($view, array(52))

olli’s picture

Status: Needs review » Needs work

oops

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.49 KB

Oh gosh, I searched already for ages. Thanks for open my eyes!

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, drupal-1934558-10.patch, failed testing.

olli’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#10: drupal-1934558-10.patch queued for re-testing.

olli’s picture

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/ArgumentDateTest.php
@@ -0,0 +1,326 @@
+  /**
+   * Stores the plugin ID used in the current test method.
+   *
+   * @var string
+   */
+  protected $usedPlugin = 'date';

Is this needed?

damiankloip’s picture

Agreed, this looks like great coverage!

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/ArgumentDateTest.phpundefined
@@ -0,0 +1,326 @@
+      'node_created_fulldate',
+      'node_created_day',
+      'node_created_month',
+      'node_created_week',
+      'node_created_year',
+      'node_created_year_month',

I guess this depends on whether this or the actual moving of the handlers gets in first.

Also, I think olli is right, that usedPlugin property doesn't seem to be used anywhere.

dawehner’s picture

FileSize
675 bytes
13.36 KB

Yeah it's totally useless.

@damiankloip
Yeah you are totally right, and I'm happy to reroll this patch once your other patch got in.

Status: Needs review » Needs work

The last submitted patch, drupal-1934558-14.patch, failed testing.

olli’s picture

+    $this->executeView($view, array('2'));
...
+    $this->executeView($view, array('5'));

Not sure why but week is now 01-53, so '02' and '05' here. Or should we ensure the padding elsewhere?

dawehner’s picture

Status: Needs work » Needs review
FileSize
971 bytes
13.36 KB

You are awesome man!

Just wondering whether we should fix the behavior in another issue? I guess for users it's probably expected that both "02" and "2" works,
but yeah this is probably out of scope of this issue.

aspilicious’s picture

Yeah 02 and 2 should be both accepted. (followup)

dawehner’s picture

So you don't have other points to review? ;)

tim.plunkett’s picture

Title: Create a test for views date handlers. » Create a test for views date handlers
Status: Needs review » Reviewed & tested by the community

Looks good to me. I agree with the change made in #18.

olli’s picture

In the follow-up, I think we would want day and month work the same way as week (both 01 and 1 are good).

Additionally, this issue made me wonder if we need to add a year handler "Year for the week" that can be used with the week handler and support sunday as the first day of week as well.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, test coverage!

Committed and pushed to 8.x. Thanks!

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