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

Comments

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 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 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
commented
StatusFileSize
new interdiff.txt1.5 KB
new drupal-1946208-3.patch14.93 KB

Yeah why not provide a better message.

dawehner’s picture

dawehner
Primary language German
commented

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

dawehner’s picture

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

tim.plunkett
he/him
Primary language English
Location Philadelphia
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
commented
Status: Needs work » Needs review
StatusFileSize
new drupal-1946208-8.patch15.22 KB

There we go.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
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
commented
StatusFileSize
new interdiff.txt664 bytes
new query_groupby_test-1946208-10.patch15.22 KB

Good point.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
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
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
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 🇪🇺🌍
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.