The CRUD tests in Drupal\views\Tests\ViewStorageTest are mostly using the default views. So we have a dependency on node, comment, etc..

CommentFileSizeAuthor
#24 vdc-1821524-24.patch8.16 KBdamiankloip
#18 vdc-1821524-18.patch8.24 KBdamiankloip
#18 interdiff.txt2.34 KBdamiankloip
#16 vdc-1821524-16.patch7.91 KBdamiankloip
#16 interdiff.txt1.34 KBdamiankloip
#11 interdiff.txt4.69 KBtim.plunkett
#11 vdc-1821524-11.patch6.87 KBtim.plunkett
#9 views-1821524-9.patch6.67 KBrlnorthcutt
#8 views-1821524-8.patch5.9 KBrlnorthcutt
#2 views-1821524-2.patch5.97 KBrlnorthcutt
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue tags: +Novice

Let's add a novice tag. (i thought that i created an issue for that yesterday as well).

rlnorthcutt’s picture

rlnorthcutt
he/him
CreditAttribution: rlnorthcutt commented
Status: Active » Needs review
FileSize
views-1821524-2.patch5.97 KB

Attached is a patch that does this, and my testing passes... hope this does the job, but please check it out and let me know.

ron

patrickd’s picture

patrickd CreditAttribution: patrickd commented

looks good but I think we should add a "block" display to the test view and keep the assertion for it

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Nice work! This generally looks pretty good. Just a few things;

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -60,6 +60,8 @@ class ViewStorageTest extends ViewTestBase {
+  protected $viewName = 'test_view_storage';

We need a docblock for this , just quickly documenting the property and it's data type.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -107,7 +109,7 @@ protected function loadTests() {
-    $expected_displays = array('default', 'page_1', 'block_1');

You can add a block display back if you like, I don't see this as a deal breaker though. It's just checking hte keys of what was loaded, so at this point you could literally add anything in there.

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -176,9 +178,9 @@ protected function createTests() {
     // Check the UUID of the loaded View.
-    $created->set('name', 'glossary_new');
+    $created->set('name', $this->viewName);
     $created->save();
-    $created_loaded = $this->loadView('glossary_new');

We don't create a new config entity here now, I think we need to change the id (name) before we save, just to keep things how they were before. So we can maybe just switch this out for creating a new view in displayTests.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Needs work
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -60,6 +60,8 @@ class ViewStorageTest extends ViewTestBase {
+  protected $viewName = 'test_view_storage';

What about just using 'test_view_storage' instead of $this->viewName, it seems to be a bit easier to understand.

rlnorthcutt’s picture

rlnorthcutt
he/him
CreditAttribution: rlnorthcutt commented

I originally started with using 'test_view_storage', but we are also overwriting it in one of the tests. So, we decided that setting the variable would be a clean way to deal with this change. Also, this makes it easier to test a specific view by changing a single line, or to copy the file to do testing on another view entirely.

I'm not tied to either approach. If you think that readability is more important than possibly reusability, then I can easily change it back.

rlnorthcutt’s picture

rlnorthcutt
he/him
CreditAttribution: rlnorthcutt commented
Status: Needs work » Needs review
FileSize
views-1821524-8.patch5.9 KB

Ok guys, a few of the changes made from above:

  1. Added docblock for $viewName
  2. Added a block display to the test
  3. Changed the view name before saving in createTests()
  4. Removed "node" from the modules list

I tried removing the other 3 modules (search, comment, taxonomy), but it continued to give me an error on lines 148 and 151. I don't see that they should be required in this test, but I haven't dug down to see where this may be happening.

Assuming the rest of the patch is ok, should I continue to try and figure out how to decouple these modules on this issue, or should we create a new one for that? My preference is for the later case - I think that this issue is clear, and that removing the other modules makes sense as another issue.

Ron

rlnorthcutt’s picture

rlnorthcutt
he/him
CreditAttribution: rlnorthcutt commented
FileSize
views-1821524-9.patch6.67 KB

Ok - Daniel helped me figure out the issue with the required modules. This section of the test is really for testing the configuration entities - which we don't need here anymore. After removing the module list and the lines of code doing this extra testing, everything else works fine.

I re-rolled the above patch to include these changes, and that should do it!

ACF’s picture

ACF CreditAttribution: ACF commented

#9: views-1821524-9.patch queued for re-testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
vdc-1821524-11.patch6.87 KB
interdiff.txt4.69 KB

Updated for the $testViews approach.
Also, on second thought, the $this->viewName was actually MORE confusing.

Status: Needs review » Needs work
Issue tags: -Novice, -VDC

The last submitted patch, vdc-1821524-11.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review

#11: vdc-1821524-11.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +VDC

The last submitted patch, vdc-1821524-11.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Yeah, that change seems fine, dawehner never liked using the property to store the view name anyway :)

Looks like the load tests have also been removed, I would be inclined to keep the load() controller tests in there in some flavour, as we are overriding the load() method on our ViewStorageController.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.34 KB
vdc-1821524-16.patch7.91 KB

This issue somehow got lost from our radar, sorry @rlnorthcutt! Your work is appreciated.

I've re rolled this patch to convert the last test (testCreateDuplicate) to use our new views_test_data view. So then we can also convert this to a unit test (Nice!). Just had to unit enable the system module so we can use the l() for the existing display tests. This also cuts the test time from ~10 seconds to 1 second :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -54,16 +54,16 @@ class ViewStorageTest extends ViewTestBase {
+      'name' => 'View storage tests',

It's so a better name now!

+++ b/core/modules/views/lib/Drupal/views/Tests/ViewStorageTest.phpundefined
@@ -176,9 +158,9 @@ protected function createTests() {
+    $created_loaded = $this->loadView('test_view_storage_new');

Just wondering why we can't use entity_load('view', 'test_view_storage_new') instead? I would suggest to not add a helper function to views.module if not needed, because people might conflict with views_get_view() which is what they actually want most of the time.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
interdiff.txt2.34 KB
vdc-1821524-18.patch8.24 KB

Yeah that is a better name :) and sure, that is a better idea just using entity_load, then we can get rid of that loadView helper, win!

Still loving the fact that these tests run in 1 second!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

This is great! Thanks rlnorthcutt and damiankloip.

Dries’s picture

Dries CreditAttribution: Dries commented

#18: vdc-1821524-18.patch queued for re-testing.

Dries’s picture

Dries CreditAttribution: Dries commented

Asked for a re-test. I don't think the patch still applies.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, vdc-1821524-18.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: Unassigned » damiankloip

I'll re roll this shortly whilst I'm on the train.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: damiankloip » Unassigned
Status: Needs work » Needs review
FileSize
vdc-1821524-24.patch8.16 KB

Yeah, we had a few changes to that file in the last few commits. This should be good now.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Yep, just a patch-context conflict, nothing actually changed.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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