Convert as many Views tests as possible to DrupalUnitTestBase

What a great change!

Also, this attempts to disable all test views by default, I'm curious to see what that breaks.

This change basically breaks some efforts of #1826602: Allow all configuration entities to be enabled/disabled so do we really have to include this in this patch, but i totally agree that this is a change, which could potentially improve the speed of the tests.

The patch includes a static class, which contains the views data, schema and actual test data,
so this code doesn't have to be duplicated.

If anyone has an idea on how to work around PHP not supporting multi-class inheritance, please chime in; a good deal of this is copy pasted from ViewTestBase.

Traits! But we can't use them yet :) So copy it is, I guess. Or make them static methods if they're some sort of helpers, haven't actually looked at the patch.

This sounds great, there are probably many more tests that can be converted :)

Uh, wrong patch? :)

This is a lot of great work!

If you just compare http://qa.drupal.org/pifr/test/391993 and http://qa.drupal.org/pifr/test/392093 we have maybe already a good performance win.

Some small things/nitpicks are below.

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FilterEqualityTest.phpundefined
@@ -63,7 +66,8 @@ function testEqual() {
+    $view = views_get_view('test_view');
+    $view->storage->newDisplay('page', 'Page', 'page_1');

@@ -114,7 +119,8 @@ function testNotEqual() {
+    $view = views_get_view('test_view');
+    $view->storage->newDisplay('page', 'Page', 'page_1');

I try to figure out why we need that exactly, so probably because we use another view?

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FilterInOperatorTest.phpundefined
@@ -7,10 +7,17 @@
+  protected $column_map = array(

Do we have a common documentation for that ones? Maybe it should be even set in the ViewUnitTestBase, as it is used in the test_view?

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FilterInOperatorTest.phpundefined
@@ -97,20 +103,17 @@ public function testFilterInOperatorSimple() {
+    $view->displayHandlers['default']->overrideOption('filters', $filters);

@@ -126,20 +129,17 @@ public function testFilterInOperatorGroupedExposedSimple() {
+    $view->displayHandlers['default']->overrideOption('filters', $filters);

It doesn't really make sense to override options for the default display :)

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FilterStringTest.phpundefined
@@ -67,19 +68,20 @@ protected function dataSet() {
+  protected function getBasicPageView() {
+    $view = views_get_view('test_view');
+
+    // In order to test exposed filters, we have to disable
+    // the exposed forms cache.
+    drupal_static_reset('views_exposed_form_cache');
+
+    $view->storage->newDisplay('page', 'Page', 'page_1');
     return $view;

Aren't we throwing the usage away anyway?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
@@ -0,0 +1,168 @@
+  /**
+   * The view to use for the test.
+   *
+   * @var \Drupal\views\ViewExecutable
+   */
+  protected $view;

Is this still needed?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
@@ -0,0 +1,168 @@
+   * @param view $view
...
+  protected function executeView($view, $args = array()) {

We could update that to enforse a ViewExecutable but yeah this is just a move.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
index d1ab4ae..65b5dee 100644
--- a/core/modules/views/lib/Drupal/views/Tests/ViewsDataTest.php

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewsDataTest.phpundefined
@@ -7,12 +7,14 @@
+use Drupal\views\Tests\ViewUnitTestBase;
...
+class ViewsDataTest extends ViewUnitTestBase {

No need for the use here.

Related: #1839134: Convert database tests to (Drupal)UnitTestBase. Tested the BasicTest before and after the patch and got down from 50s to 20. So the difference isn't as huge as with the database tests but I'm also testing on a slower system this time and the views tests obviously need to enable way more modules.

Still, this is very nice :)

+++ b/core/modules/views/lib/Drupal/views/Tests/BasicTest.phpundefined
@@ -7,10 +7,12 @@
 namespace Drupal\views\Tests;
 
+use Drupal\views\Tests\ViewUnitTestBase;

The use isn't necessary?

Had a quick look at the failing test but didn't see anything obvious other than that the row_test plugin is only used in one other test and that's a UI test, so that at least explains why this is the only one that is failing because of it :)

I would say this patch looks 'mint' now :)

For me this is about an initial conversion of what can be easily done now, so it happily ticks that box!

Looks good to me as well. Did some profiling and noticed that we're importing *many* default config files for all these views and opened #1851234: Slow yaml parsing slows down tests which load/save many config files (e.g. views).

This isn't quite up to snuff docs-wise... a few minor bits, then hopefully we can get this lovely patch in.

+++ b/core/modules/views/lib/Drupal/views/Tests/Handler/FieldWebTest.phpundefined
@@ -2,23 +2,26 @@
+class FieldWebTest extends HandlerTestBase {
 
-class FieldTest extends HandlerTestBase {

Everywhere else we're replacing HandlerTestBase with ViewsUnitTestBase. Why not here too?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewTestData.phpundefined
@@ -0,0 +1,212 @@
+class ViewTestData {

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
@@ -0,0 +1,155 @@
+  protected function schemaDefinition() {
+    return ViewTestData::schemaDefinition();

I love this. :D

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
@@ -0,0 +1,155 @@
+/**
+ * Abstract class for views testing.
+ */
+abstract class ViewUnitTestBase extends DrupalUnitTestBase {

That's a copy/pasta from ViewsTestBase. We should disambiguate these two classes, since they're for different things.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
@@ -0,0 +1,155 @@
+  protected function assertIdenticalResultsetHelper($view, $expected_result, $column_map, $message, $assert_method) {

Missing PHPDoc.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewUnitTestBase.phpundefined
@@ -0,0 +1,155 @@
+  /**
+   * Helper function: order an array of array based on a column.
+   */
+  protected function orderResultSet($result_set, $column, $reverse = FALSE) {

Missing @param/@return.

Thought there was one other one but I forget now. :)

I'll add some docs polish *yawn* tomorrow morning. :)

For the record, the docblock for WebTestBase is:

Test case for typical Drupal tests.

So I think @tim.plunkett can be forgiven for copying the test base class docs as they were. ;)

We can file a followup to rename ViewTestBase to ViewWebTestBase for clarity. (And for the WebTestBase docblock.)

I'll focus on cleaning up the docs in ViewTestBase, View[Web]TestBase, and ViewTestData. There are a number of missing docblocks and other docs style errors in the test classes that we're moving to DrupalUnitTestBase here, but changing that is likely out of scope here and can be done later in the release cycle as part of our general codebase cleanup.

Edited to clarify what I meant.

Oh. I keep typing ViewsTestBase, etc., when it is ViewTestBase. Why is it ViewTestBase?

Hm. I notice in documenting this that there are a lot of methods on ViewTestBase and ViewUnitTestBase (I keep having to delete the s) that are 100% identical, unless I've missed some difference. Could we move these into ViewTempTestBase extends TestBase, and do ViewUnitTestBase extends DrupalUnitTestBase extends ViewTempTestBase? And then in a followup rename ViewTempTestBase to ViewTestBase and ViewTestBase to ViewWebTestBase.

With this patch, the two classes are ViewTestBase and ViewUnitTestBase (the opposite of what is mentioned above). Just FYI.

Hm? You must have replied to my post while I was neurotically editing typos? :)

@xjm: Only if you first implement multiple inheritance support for PHP :) In PHP 5.4, it would be possible to use a trait for this, but we can't do that yet. So we really only have two options: static methods or duplicating it.

Oh, like it says in #9. I read the issue, I swear. Yup.

Docs docs docs.

gitiocy again.

For the record, #39 is a bit bigger because it has FieldTest to FieldWebTest as a full diff rather than a rename, but it's the same code. The interdiff in #38 shows the changes.

xpost

#39: vdc-1828642-really-for-reals-39.patch queued for re-testing.

Testbot :(

Bot issues. #39 is green and RTBC.

Let's hear it for more fasterererer tests! :D

Committed and pushed to 8.x. Thanks!