Problem/Motivation

Most of \Drupal\Tests\taxonomy\Functional\Views\TaxonomyDefaultArgumentTest can actually be a kernel test and as such be executed faster.

Proposed resolution

Convert all beside \Drupal\Tests\taxonomy\Functional\Views\TaxonomyDefaultArgumentTest::testTermTitleEscaping to a kernel test

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 interdiff-2910807-6-11.txt4 KBnginex
#11 2910807-11.patch11.84 KBnginex
#6 interdiff-2910807-5-6.txt592 bytesnginex
#6 2910807-6.patch12.13 KBnginex
#5 2910807-5.patch12.2 KBnginex
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

ApacheEx’s picture

ApacheEx CreditAttribution: ApacheEx commented
Issue tags: +LutskGSW18

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.

nginex’s picture

nginex CreditAttribution: nginex at Drupal Ukraine Community, AnyforSoft commented
Assigned: Unassigned » nginex
Status: Active » Needs work

I'm going to convert this test on Drupal Global Sprint Weekend in Lutsk 27.01.18

nginex’s picture

nginex CreditAttribution: nginex at Drupal Ukraine Community, AnyforSoft commented
Assigned: nginex » Unassigned
Status: Needs work » Needs review
FileSize
2910807-5.patch12.2 KB

1. The patch has been attached.
2. TaxonomyDefaultArgumentTest has been converted to Kernel/Views directory (except method testTermTitleEscaping).
3. A new abstract class TaxonomyTestBase has been created to be used with ViewsKernelTestBase.

nginex’s picture

nginex CreditAttribution: nginex at Drupal Ukraine Community, AnyforSoft commented
FileSize
2910807-6.patch12.13 KB
interdiff-2910807-5-6.txt592 bytes

Reverted some changes from old TaxonomyDefaultArgumentTest.

The last submitted patch, 5: 2910807-5.patch, failed testing. View results

nginex’s picture

nginex CreditAttribution: nginex at Drupal Ukraine Community, AnyforSoft commented

Any updates here?

The previous patch #5 was failed and the new patch #6 seems to work.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented 
  1. +++ b/core/modules/taxonomy/tests/src/Kernel/Views/TaxonomyDefaultArgumentTest.php
@@ -0,0 +1,91 @@
+    $request = Request::create($this->nodes[0]->url());
+    $request->server->set('SCRIPT_NAME', $GLOBALS['base_path'] . 'index.php');
+    $request->server->set('SCRIPT_FILENAME', 'index.php');
+
+    $response = $this->container->get('http_kernel')
+      ->handle($request, HttpKernelInterface::SUB_REQUEST);
+    $view->setRequest($request);
+    $view->setResponse($response);
...
+    $request = Request::create($this->nodes[0]->url());
+    $request->server->set('SCRIPT_NAME', $GLOBALS['base_path'] . 'index.php');
+    $request->server->set('SCRIPT_FILENAME', 'index.php');
+
+    $response = $this->container->get('http_kernel')->handle($request, HttpKernelInterface::SUB_REQUEST);
+    $view->setRequest($request);
+    $view->setResponse($response);
...
+    $request = Request::create($this->term1->url());
+    $request->server->set('SCRIPT_NAME', $GLOBALS['base_path'] . 'index.php');
+    $request->server->set('SCRIPT_FILENAME', 'index.php');
+
+    $response = $this->container->get('http_kernel')->handle($request, HttpKernelInterface::SUB_REQUEST);
+    $view->setRequest($request);
+    $view->setResponse($response);
+    $view->initHandlers();

    We are doing the same thing 3 times here. Do you think it would be helpful to add it to a helper method here? 

  2. +++ b/core/modules/taxonomy/tests/src/Kernel/Views/TaxonomyTestBase.php
@@ -0,0 +1,177 @@
+/**
+ * Base class for all taxonomy tests.
+ */
+abstract class TaxonomyTestBase extends ViewsKernelTestBase {

    This documentation seems a little bit overly generic ;)

nginex’s picture

nginex CreditAttribution: nginex at Drupal Ukraine Community, AnyforSoft commented
Assigned: Unassigned » nginex
Status: Needs review » Needs work

I agree with you, it should be refactored, I'm going to do it.

nginex’s picture

nginex CreditAttribution: nginex at Drupal Ukraine Community, AnyforSoft commented
Assigned: nginex » Unassigned
Status: Needs work » Needs review
FileSize
2910807-11.patch11.84 KB
interdiff-2910807-6-11.txt4 KB

This is a refactoring to avoid duplication of the code.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

The refactor in #11 fixes the remarks @dawehner had in #9, the documentation also has improved.

I don't really see the reason for the base class to be added when there is only one class that uses it, but since @dawehner didn't seem to mind in #9, I think this is good to go.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented 
+++ b/core/modules/taxonomy/tests/src/Kernel/Views/TaxonomyTestBase.php
@@ -0,0 +1,184 @@
+  protected function createTerm(array $settings = []) {

We have ContentTypeCreationTrait and NodeCreationTrait, would this be useful in a standalone trait? Follow up?

Adding review credits

  • larowlan committed 3a44b43 on 8.6.x 
    Issue #2910807 by nginex, dawehner: Convert \Drupal\Tests\taxonomy\...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 3a44b43 and pushed to 8.6.x.

Cherry-picked as be1f0d7 and pushed to 8.5.x.

  • larowlan committed be1f0d7 on 8.5.x 
    Issue #2910807 by nginex, dawehner: Convert \Drupal\Tests\taxonomy\...

Status: Fixed » Closed (fixed)

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