Problem/Motivation

The test in 

 \Drupal\views\Tests\Handler\HandlerTest::testBreakString()

could be moved to a unit test, which is fay faster to execute, and better integrated with IDEs etc.

Proposed resolution

Move the code to the phpunit test, but use a dataprovider, see https://phpunit.de/manual/3.7/en/appendixes.annotations.html#appendixes....

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#64 2430835-64.patch9.27 KBgeertvd
#63 2430835-63.patch0 bytesgeertvd
#63 interdiff-2430835-61-63.txt554 bytesgeertvd
#61 2430835-61.patch9.29 KBgeertvd
#61 interdiff-2430835-59-61.txt708 bytesgeertvd
#59 2430835-59.patch9.38 KBgeertvd
#59 interdiff-2430835-53-59.txt1.52 KBgeertvd
#53 interdiff.txt1.15 KBjoshi.rohit100
#53 move-views-test-2430835-53.patch9.66 KBjoshi.rohit100
#51 move-views-test-2430835-51.patch9.63 KBottlik
#49 move-views-test-2430835-49.patch9.86 KBottlik
#47 move-views-test-2430835-47.patch9.71 KBottlik
#44 move-views-test-2430835-44.patch9.72 KBottlik
#42 move-views-test-2430835-42.patch9.49 KBottlik
#40 move-views-test-2430835-40.patch7.5 KBottlik
#37 move-views-test-2430835-37.patch7.5 KBottlik
#32 move-views-test-2430835-32.patch7.55 KBottlik
#28 move-views-test-2430835-28.patch7.5 KBottlik
#25 move-views-test-2430835-25.patch7.55 KBottlik
#19 2430835_move_views_test_16.patch7.61 KBharshil.maradiya
#18 2430909_hook_theme_suggestions_10.patch1.54 KBharshil.maradiya
#15 move-views-test-2430835-15.patch7.51 KBottlik
#13 move-views-test-2430835-13.patch7.37 KBottlik
#10 2430835_move_views_test.patch8.6 KBharshil.maradiya
#7 2430835_move_views_test.patch7.7 KBharshil.maradiya
#3 2430835_move_test.patch9.65 KBharshil.maradiya

Comments

ottlik’s picture

ottlik commented

ottlik will be working on this issue as of Feb. 25, 2015.

ottlik’s picture

ottlik commented
Assigned: Unassigned » ottlik
harshil.maradiya’s picture

harshil.maradiya commented
StatusFileSize
new 2430835_move_test.patch9.65 KB

as per instruction test is moved to destination class

harshil.maradiya’s picture

harshil.maradiya commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 2430835_move_test.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

@ottlik @harshil.maradiya
It would be cool if you could read my issue summary, ... there I suggested to use a @dataProvider

+++ b/core/modules/views/src/Tests/Handler/HandlerTest.php
@@ -88,102 +88,7 @@ function testFilterInOperatorUi() {
-
+ ¶

Quick nitpick, there is a whitespace left here :)

harshil.maradiya’s picture

harshil.maradiya commented
StatusFileSize
new 2430835_move_views_test.patch7.7 KB

@dawehner thank you so much
Now i have implemented patch using data provider

harshil.maradiya’s picture

harshil.maradiya commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2430835_move_views_test.patch, failed testing.

harshil.maradiya’s picture

harshil.maradiya commented
StatusFileSize
new 2430835_move_views_test.patch8.6 KB

Fixing patch

harshil.maradiya’s picture

harshil.maradiya commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: 2430835_move_views_test.patch, failed testing.

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-13.patch7.37 KB

Tried to apply patch, got errors:
"Patch does not apply", which I didn't understand.
"Looking for }", but AFAICT the brackets are balanced.
Whitespace errors which of course I could fix. :)
Thought I'd upload in hopes that you (harshil.maradiya and/or dawehner) might find it fixable.

Status: Needs review » Needs work

The last submitted patch, 13: move-views-test-2430835-13.patch, failed testing.

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-15.patch7.51 KB

Moved a bracket. Checked syntax on phpcodechecker.com, which said syntax was ok.

Status: Needs review » Needs work

The last submitted patch, 15: move-views-test-2430835-15.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

Thank you for working on it!

  1. +++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -91,11 +91,64 @@ public function testGetEntityTypeForFieldWithRelationship() {
+  public function testBreakString($array0, $op0, $string0) {

    Some suggestions for a better variable naming: $expected_splitted_string, $expected_operator, $input_string 

  2. +++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -91,11 +91,64 @@ public function testGetEntityTypeForFieldWithRelationship() {
+  public function breakstringProvider()
+  {

    Nitpicks: We do have the { at the end of the function line. 

  3. +++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -91,11 +91,64 @@ public function testGetEntityTypeForFieldWithRelationship() {
+      array(array('word1', 'word2', 'word'), 'or', 'word1+word2+word'), ¶
...
+      array(array('wõrd1', 'wõrd2', 'wõrd'), 'or', 'wõrd1+wõrd2+wõrd'),   ¶
...
+    );  ¶

    Nitpicks: trailing whitespace 

  4. +++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -91,11 +91,64 @@ public function testGetEntityTypeForFieldWithRelationship() {
 
-}
+  }
+
+
+
 

    Please don't change this here. 

+++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -91,11 +91,64 @@ public function testGetEntityTypeForFieldWithRelationship() {
+  /**
+   * Tests the breakString method.
+   */
...
+  /**
+   * @dataProvider breakstringProvider
+   */

This @dataProvider has to move to the documentation of testBreakString itself to work.

harshil.maradiya’s picture

harshil.maradiya commented
StatusFileSize
new 2430909_hook_theme_suggestions_10.patch1.54 KB

Fixing test cases

harshil.maradiya’s picture

harshil.maradiya commented
StatusFileSize
new 2430835_move_views_test_16.patch7.61 KB

@dawehner thanks for your comment i have fixed failure test as well as ur all comments
applying rite patch

harshil.maradiya’s picture

harshil.maradiya commented
Status: Needs work » Needs review

The last submitted patch, 18: 2430909_hook_theme_suggestions_10.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

There is still whitespace

+++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -91,11 +91,63 @@ public function testGetEntityTypeForFieldWithRelationship() {
+    return array(
+      // Test ors.

In case you want to, feel free to convert it to use the short array syntax. IMHO this would be way better to read.

The last submitted patch, 19: 2430835_move_views_test_16.patch, failed testing.

harshil.maradiya’s picture

harshil.maradiya commented

@dawehner
Thanks for the quick response but in my ide i am not able to see any any white space and would yu please help me out with why test cases are failing

Thanks & regards
Harshil Maradiya

ottlik’s picture

ottlik commented
StatusFileSize
new move-views-test-2430835-25.patch7.55 KB

Moved "@dataProvider". Improved variable names. Moved opening function bracket.
Thanks for helping!

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -91,11 +91,60 @@ public function testGetEntityTypeForFieldWithRelationship() {
-}
+  }
+
+
+
 

Just curious about this change, do we really need it?

ottlik’s picture

ottlik commented

No need for that change at all. It crept in as I was rearranging the closing brackets. Failed to notice it in the diff file or I would have changed it back.

ottlik’s picture

ottlik commented
StatusFileSize
new move-views-test-2430835-28.patch7.5 KB

Eliminated the unnecessary change.

harshil.maradiya’s picture

harshil.maradiya commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 28: move-views-test-2430835-28.patch, failed testing.

The last submitted patch, 25: move-views-test-2430835-25.patch, failed testing.

ottlik’s picture

ottlik commented
StatusFileSize
new move-views-test-2430835-32.patch7.55 KB

Added debug($handler) and debug($handler-operator), hoping to see what's going wrong with the tests. Anyone have any tips on debugging?

ottlik’s picture

ottlik commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: move-views-test-2430835-32.patch, failed testing.

harshil.maradiya’s picture

harshil.maradiya commented

Ws trying to reproduce on local but tests are not running no clue why these tests are failing

dawehner’s picture

dawehner
Primary language German
commented

You want to use $handler->value in the first assertEquals ... note: I would use a different variable name, maybe $result

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-37.patch7.5 KB

Changed $handler to $result, line 99, changed $handler to $result->value, line 100, and changed $handler->operator to $result->operator, line 101. Also removed debug lines.

Status: Needs review » Needs work

The last submitted patch, 37: move-views-test-2430835-37.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

Cool, now you just have to figure out the remaining 2 failures.

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-40.patch7.5 KB

Changed first assertEquals back to assertEqualValue, so changed $result->value back to $result, thinking that the fail was in the "(int)" tests. Can't imagine why I changed it in the first place, as the original had "assertEqualValue".

Status: Needs review » Needs work

The last submitted patch, 40: move-views-test-2430835-40.patch, failed testing.

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-42.patch9.49 KB

Moved function assertEqualValue from HandlerTest.php to HandlerBaseTest.php.

Status: Needs review » Needs work

The last submitted patch, 42: move-views-test-2430835-42.patch, failed testing.

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-44.patch9.72 KB

Included bootstrap.inc file so t function would be visible.

Status: Needs review » Needs work

The last submitted patch, 44: move-views-test-2430835-44.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented

Changed first assertEquals back to assertEqualValue, so changed $result->value back to $result, thinking that the fail was in the "(int)" tests. Can't imagine why I changed it in the first place, as the original had "assertEqualValue".

But was there an actual reason, why do you think we need it?

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-47.patch9.71 KB

Changed erroneous include line to a correct (I hope) require line so includes/bootstrap.inc can be found. Alternatively I could scrap the error message from the original assertEqualValue function.
@dawehner The fails were in one (if counting starts from 1) or both (if from 0) of the tests where the (int) cast was used, so I was thinking that assertEqualValue might have a slightly different criterion for equality than assertEquals. Otherwise the fails are pretty mysterious so far.

Status: Needs review » Needs work

The last submitted patch, 47: move-views-test-2430835-47.patch, failed testing.

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-49.patch9.86 KB

Having trouble figuring out the absolute path to the includes directory, so copied the "use" namespaces from the HandlerTest file to the HandlerBaseTest file, hoping that might do what is needed to make the t function definition visible.

Status: Needs review » Needs work

The last submitted patch, 49: move-views-test-2430835-49.patch, failed testing.

ottlik’s picture

ottlik commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-51.patch9.63 KB

Copied the t() function definition and removed the "use" namespaces.

Status: Needs review » Needs work

The last submitted patch, 51: move-views-test-2430835-51.patch, failed testing.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented
Status: Needs work » Needs review
StatusFileSize
new move-views-test-2430835-53.patch9.66 KB
new interdiff.txt1.15 KB

Removed fatal error.

Status: Needs review » Needs work

The last submitted patch, 53: move-views-test-2430835-53.patch, failed testing.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented

PHP error is gone :)

harshil.maradiya’s picture

harshil.maradiya commented

thanks for that but the test cases are still failing

snehi queued 53: move-views-test-2430835-53.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +rc eligible

This issue would still be nice to land as its "rc eligible"

geertvd’s picture

geertvd commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2430835-53-59.txt1.52 KB
new 2430835-59.patch9.38 KB
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -91,8 +91,74 @@ public function testGetEntityTypeForFieldWithRelationship() {
+    $s1 = $this->randomMachineName();
+    // Generate three random numbers which can be used below.
+    $n1 = rand(0, 100);
+    $n2 = rand(0, 100);
+    $n3 = rand(0, 100);

Let's just use non random values instead

geertvd’s picture

geertvd commented
StatusFileSize
new interdiff-2430835-59-61.txt708 bytes
new 2430835-61.patch9.29 KB
dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/views/tests/src/Unit/Plugin/HandlerBaseTest.php
@@ -123,10 +123,9 @@ public function testBreakString($expected_split_string, $expected_operator, $inp
     $s1 = $this->randomMachineName();

This is still kinda random :)

geertvd’s picture

geertvd commented
StatusFileSize
new interdiff-2430835-61-63.txt554 bytes
new 2430835-63.patch0 bytes

Can't come up with any original non-random values today :)

geertvd’s picture

geertvd commented
StatusFileSize
new 2430835-64.patch9.27 KB

empty patch

The last submitted patch, 63: 2430835-63.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Thank you!

The last submitted patch, 63: 2430835-63.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: 2430835-64.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mradcliffe’s picture

mradcliffe
he/him
Primary language English
commented
Issue tags: -Novice, -rc eligible +Needs issue summary update

This issue would make an API break, and needs to be re-evaluated.,

dawehner’s picture

dawehner
Primary language German
commented

Mh, there is no base class we change. I don't see how this is an API break.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +stale-issue-cleanup

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

smustgrave commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

This file has drupalGet() call which can't be done in unit tests so believe this is no longer valid. Am saving credit though.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.