It seems to be helpful for various reasons that there are as less tests as possible which are coupled with nodes.

Let's start with the QueryGroupByTest.

CommentFileSizeAuthor
#10 query_groupby_test-1946208-10.patch15.22 KBdawehner
#10 interdiff.txt664 bytesdawehner
#8 drupal-1946208-8.patch15.22 KBdawehner
#3 drupal-1946208-3.patch14.93 KBdawehner
#3 interdiff.txt1.5 KBdawehner
#1 drupal-1946208-1.patch14.45 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
drupal-1946208-1.patch14.45 KB

Various different places needed work, like the entity_test views integration, as this has been outdated.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

This generally looks great already.

+++ b/core/modules/views/lib/Drupal/views/Tests/QueryGroupByTest.phpundefined
@@ -58,86 +65,104 @@ public function testAggregateCount() {
+    $this->assertEqual($types['name1'], 4);
+    $this->assertEqual($types['name2'], 3);
...
+    $this->assertEqual($results['name1'], $values[0]);
+    $this->assertEqual($results['name2'], $values[1]);

These would prob be better with an assertion message too.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt1.5 KB
drupal-1946208-3.patch14.93 KB

Yeah why not provide a better message.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

#3: drupal-1946208-3.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Test suite performance
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

If this still passes, it looks great. Provides just as good coverage and is faster, AND is decoupled from node.module.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-1946208-3.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
drupal-1946208-8.patch15.22 KB

There we go.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/modules/views/lib/Drupal/views/Tests/QueryGroupByTest.phpundefined
@@ -57,87 +66,106 @@ public function testAggregateCount() {
+  public function GroupByTestHelper($aggregation_function, $values) {
...
-    $this->GroupByTestHelper('max', array(4, 7));
...
+    $this->groupByTestHelper('max', array(4, 7));

I know it used to be camelcased wrong, but let's just fix it here.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt664 bytes
query_groupby_test-1946208-10.patch15.22 KB

Good point.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Even better than the last time I RTBC'd it :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented 
+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
@@ -222,6 +222,7 @@ protected function executeView($view, $args = array()) {
     $view->execute();
+    $this->verbose('<pre>Executed view: ' . ((string) $view->build_info['query']) . '</pre>');
   }

Is this intentional? Looks unrelated.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

It was intended as it was helpful to debug this. Views currently has two base test classes, which both has the executeView method:

ViewUnitTestBase()

  /**
   * Executes a view with debugging.
   *
   * @param \Drupal\views\ViewExecutable $view
   *   The view object.
   * @param array $args
   *   (optional) An array of the view arguments to use for the view.
   */
  protected function executeView($view, $args = array()) {
    $view->setDisplay();
    $view->preExecute($args);
    $view->execute();
  }

ViewTestBase (web tests):

  /**
   * Executes a view with debugging.
   *
   * @param \Drupal\views\ViewExecutable $view
   *   The view object.
   * @param array $args
   *   (optional) An array of the view arguments to use for the view.
   */
  protected function executeView($view, $args = array()) {
    $view->setDisplay();
    $view->preExecute($args);
    $view->execute();
    $this->verbose('<pre>Executed view: ' . ((string) $view->build_info['query']) . '</pre>');
  }
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 1ea2d22 and pushed to 8.x. Thanks!

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