Expand test coverage for ViewExecutable() [#1866804]

Comment	File	Size	Author
#13	1866804-13.patch	12.08 KB	damiankloip

#11	1866804-11.patch	12.1 KB	damiankloip

#11	interdiff.txt	1.74 KB	damiankloip
#9	1866804-9.patch	12.12 KB	damiankloip

#9	interdiff.txt	4.22 KB	damiankloip
#6	vdc-1866804-6.patch	10.01 KB	damiankloip

#5	vdc-1866804-5.patch	9.98 KB	damiankloip

#4	vdc-1866804-4.patch	10.01 KB	damiankloip

#2	vdc-1866804-2.patch	9.94 KB	damiankloip

#1	vdc-1866804.patch	8.88 KB	damiankloip

Comment #1

damiankloip CreditAttribution: damiankloip commented 14 December 2012 at 15:24

Status:

Active

» Needs review

File	Size
vdc-1866804.patch	8.88 KB

Here is a patch to kick things off.

Log in or register to post comments

Comment #2

damiankloip CreditAttribution: damiankloip commented 14 December 2012 at 16:17

File	Size
vdc-1866804-2.patch	9.94 KB

Converted the tests to ViewUnitTestBase and added another test (from @todo of last patch) for checking the usesPager() method.

Log in or register to post comments

Comment #3

dawehner

German

CreditAttribution: dawehner commented 14 December 2012 at 16:28

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -140,19 +161,58 @@ public function testDisplays() {
+    $view->setDisplay('invalid');
+    $this->assertEqual($view->current_display, 'default', 'If setDisplay is called with an invalid display id the default display should be used.');
+    $this->assertEqual(spl_object_hash($view->display_handler), spl_object_hash($view->displayHandlers['default']));

cool!

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -140,19 +161,58 @@ public function testDisplays() {
+    $this->assertFalse($view->use_ajax);
...
+    $this->assertTrue($view->use_ajax);

As a follow up we might want getUseAjax but sure this is out of scope here.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -140,19 +161,58 @@ public function testDisplays() {
+    $this->assertNull($view->usePager());

We could document in the assertion message that the test_executable_displays view has the none pager documented.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -140,19 +161,58 @@ public function testDisplays() {
+    $view->setOffset(5);
+    $this->assertEqual($view->getOffset(), 5);

What about using random integers?

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -140,19 +161,58 @@ public function testDisplays() {
+      'views_test_data' => true,
+      '#global' => true,

That's not true, it should be TRUE ;)

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -140,19 +161,58 @@ public function testDisplays() {
+  public function testDestroy() {

Tim patched in another issue to protected

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -202,21 +267,36 @@ public function testViewsHandlerTypes() {
+      $this->assertTrue(array_filter($validate, $match), "Error message found for $id display");

Shouldn't we use format_string instead (not sure).

Log in or register to post comments

Comment #4

damiankloip CreditAttribution: damiankloip commented 14 December 2012 at 16:56

File	Size
vdc-1866804-4.patch	10.01 KB

Thanks dawehner!

Tim patched in another issue to protected

Do you know which patch? Do you think I should revert this one then? Seems weird in a cleanup of this file thought :)

I'm not sure about exactly when to use the format_string() method. I like going with a normal string as it's one less function call, and every little helps.

Sure, we should create a follow up to add the getUseAJAX method.

Log in or register to post comments

Comment #5

damiankloip CreditAttribution: damiankloip commented 14 December 2012 at 17:22

File	Size
vdc-1866804-5.patch	9.98 KB

Removed the @todo I todoed already and added the format_string().

Log in or register to post comments

Comment #6

damiankloip CreditAttribution: damiankloip commented 14 December 2012 at 17:27

File	Size
vdc-1866804-6.patch	10.01 KB

Sorry, going mad, with the format_string change too.

Log in or register to post comments

Comment #7

damiankloip CreditAttribution: damiankloip commented 14 December 2012 at 17:43

I created a follow up here for the getter: #1866910: Add an ajaxEnabled() method for ViewExecutable

Log in or register to post comments

Comment #8

damiankloip CreditAttribution: damiankloip commented 14 December 2012 at 20:41

It's also worth noting, switching this to use ViewUnitTestBase cut the test time from ~45 secs to ~13 secs. Not bad!

Log in or register to post comments

Comment #9

damiankloip CreditAttribution: damiankloip commented 14 December 2012 at 21:53

File	Size
interdiff.txt	4.22 KB
1866804-9.patch	12.12 KB

Added some more tests for initQuery, get/setResponse, generateItemId, getPath, getUrl, get/setTitle methods.

Log in or register to post comments

Comment #10

dawehner

German

CreditAttribution: dawehner commented 16 December 2012 at 20:22

Great work!

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -208,6 +219,44 @@ public function testPropertySettings() {
+    $path = $this->randomString(10);

any reason to use randomString and not randomName? randomString is a great source for potential random test failures, but yeah actually for paths it shouldn't be different.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -208,6 +219,44 @@ public function testPropertySettings() {
+    $url = $this->randomString(10);
...
+    $arg1 = $this->randomString(10);
...
+    $title = $this->randomString(10);

Any special reason to use randomString(10) and not randomString()?

Log in or register to post comments

Comment #11

damiankloip CreditAttribution: damiankloip commented 17 December 2012 at 08:22

File	Size
interdiff.txt	1.74 KB
1866804-11.patch	12.1 KB

Yep, makes sense. I have changed those. I think that is the actual calls you meant?

Log in or register to post comments

Comment #12

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 19 December 2012 at 16:06

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -2,21 +2,24 @@
-class ViewExecutableTest extends ViewTestBase {
+class ViewExecutableTest extends ViewUnitTestBase {

+1!

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.phpundefined
@@ -55,7 +51,20 @@ class ViewExecutableTest extends ViewTestBase {
+    'parent_views'

Missing trailing comma

Otherwise, looks great!