Problem/Motivation

Both ViewUnitTestBase and ViewTestBase has quite a bunch of assertIdenti* methods, this is unnecessary duplication.

Proposed resolution

Extract them into a trait, probably take the one of ViewUnitTestBase as it provides more information.
Use this new trait as part of ViewUnitTestBase and ViewTestBase

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because it just removes maintaining overhead
Issue priority Normal because it makes normal views tests easier to debug
Disruption No disruption at all
CommentFileSizeAuthor
#8 2402827-8.patch15.54 KBkgoel
#6 interdiff.txt1.52 KBkgoel
#6 2402827-6.patch15.53 KBkgoel
#1 2402827-1.patch15.57 KBkgoel

Comments

kgoel’s picture

kgoel commented
Status: Active » Needs review
StatusFileSize
new 2402827-1.patch15.57 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2402827-1.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Version: 8.1.x-dev » 8.0.x-dev

Meh

Status: Needs work » Needs review

kgoel queued 1: 2402827-1.patch for re-testing.

dawehner’s picture

dawehner
Primary language German
commented

Nice, first time pass!

  1. +++ b/core/modules/views/src/Tests/ViewUnitTestBaseTrait.php
@@ -0,0 +1,132 @@
+/**
+ * Provides a class for assertIdenticalResultset, assertNotIdenticalResultset,
+ * and assertIdenticalResultsetHelper functions.

    Mh, how can we make that fit into 80 chars? Provide some assertions to check for the expected result of a view? 

  2. +++ b/core/modules/views/src/Tests/ViewUnitTestBaseTrait.php
@@ -0,0 +1,132 @@
+trait ViewUnitTestBaseTrait {

    What about naming it ViewResultAssertionTrait

kgoel’s picture

kgoel commented
StatusFileSize
new 2402827-6.patch15.53 KB
new interdiff.txt1.52 KB

Status: Needs review » Needs work

The last submitted patch, 6: 2402827-6.patch, failed testing.

kgoel’s picture

kgoel commented
Status: Needs work » Needs review
StatusFileSize
new 2402827-8.patch15.54 KB

Forgot to change the name of the file when I changed trait name :((

dawehner’s picture

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

Thank you!

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Reviewed & tested by the community » Needs work

Small nit.

+++ b/core/modules/views/src/Tests/ViewResultAssertionTrait.php
@@ -0,0 +1,131 @@
+   * @param string $message
+   *   (optional) A custom message to display with the assertion. Defaults to
+   *   'Identical result set.'
...
+   * @param string $message
+   *   (optional) A custom message to display with the assertion. Defaults to
+   *   'Non-identical result set.'
...
+   * @param string $message
+   *   (optional) The message to display with the assertion.

Me thinks these doc blocks should swap. The 1st/2nd should be the 3rd and the 3rd should have the wording from the 1st/2nd. Or just take out the default text 'Identical result set.' comment entirely. It doesn't add much.

kgoel’s picture

kgoel commented
Status: Needs work » Reviewed & tested by the community

Spoke with @heddn -

Moving this back to RTBC since $message is same in docblock for assertIdenticalResultsetHelper method in ViewTestBase and ViewUnitTestBase.

alexpott’s picture

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

Committed 9a2b86b and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 9a2b86b on 8.0.x 
    Issue #2402827 by kgoel: Extract ViewUnitTestBase and ViewTestBase::...

Status: Fixed » Closed (fixed)

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