Problem/Motivation

Drupal\Tests\taxonomy\Functional\EfqTest doesn't make any HTTP request.

Proposed resolution

  • Convert it to a kernel test.
  • Move TaxonomyTestTrait from tests/src/Functional to tests/src/Traits as is used by all kind of tests.
  • Rename it to a more meaningful name.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#17 3041030-17.patch13.65 KBKrzysztof Domański
#17 interdiff-13-17.txt648 bytesKrzysztof Domański
#13 3041030-13.patch13.02 KBKrzysztof Domański
#13 interdiff-10-13.txt2.73 KBKrzysztof Domański
#10 3041030-10.patch13.47 KBclaudiu.cristea
#10 3041030-10.interdiff.txt697 bytesclaudiu.cristea
#8 3041030-8.patch13.44 KBclaudiu.cristea
#8 3041030-8.interdiff.txt1.91 KBclaudiu.cristea
#6 3041030-6.patch13.99 KBclaudiu.cristea
#6 3041030-6.interdiff.txt830 bytesclaudiu.cristea
#5 3041030-5.patch14 KBclaudiu.cristea
#5 3041030-5.interdiff.txt1 KBclaudiu.cristea
#2 3041030-2.patch11.38 KBclaudiu.cristea
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Active » Needs review
Issue tags: +Testing system, +Test suite performance, +Performance, +functional2kernel
FileSize
3041030-2.patch11.38 KB

Patch.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext commented 
+++ b/core/modules/taxonomy/tests/src/Kernel/Views/TaxonomyFieldVidTest.php
similarity index 69%
rename from core/modules/taxonomy/tests/src/Functional/TaxonomyTestTrait.php

rename from core/modules/taxonomy/tests/src/Functional/TaxonomyTestTrait.php
rename to core/modules/taxonomy/tests/src/Traits/TaxonomyTestTrait.php

I'm pretty sure we can't do that without a BC layer.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Parent issue: » #3041700: [META] Convert some tests into Kernel or Unit tests
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
FileSize
3041030-5.interdiff.txt1 KB
3041030-5.patch14 KB

@jibran, you're right. Here's the BC layer.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
FileSize
3041030-6.interdiff.txt830 bytes
3041030-6.patch13.99 KB

Fixed coding standards.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at ezCompany commented
Status: Needs review » Needs work

Just some notes, nothing blocking I think.

  1. +++ b/core/modules/system/tests/src/Functional/Module/PrepareUninstallTest.php
index eb9e5e983b..7a980bd148 100644
--- a/core/modules/taxonomy/src/Tests/TaxonomyTestBase.php

--- a/core/modules/taxonomy/src/Tests/TaxonomyTestBase.php
+++ b/core/modules/taxonomy/src/Tests/TaxonomyTestBase.php

+++ b/core/modules/taxonomy/src/Tests/TaxonomyTestBase.php
@@ -6,7 +6,7 @@
-use Drupal\Tests\taxonomy\Functional\TaxonomyTestTrait;
+use Drupal\Tests\taxonomy\Traits\TaxonomyTestTrait;

    Not strictly necessary since it's updating a class that is deprecated itself. 

  2. +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTestTrait.php
@@ -2,58 +2,20 @@
+@trigger_error('The ' . __NAMESPACE__ . '\TaxonomyTestTrait is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\taxonomy\Traits\TaxonomyTestTrait. See https://www.drupal.org/node/3041703.', E_USER_DEPRECATED);

    See #3024461: Adopt consistent deprecation format for core and contrib deprecation messages for the new way of writing these, since this has not been officially approved, not setting it back now (but might have to update this is that lands first)

Went through the conversion and everything checks out.

The only part I'm missing that is mentioned in the IS is: "Rename it to a more meaningful name", which I think is worth doing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs work » Needs review
Issue tags: -tests convert, -Testing system, -Performance, -functional2kernel
FileSize
3041030-8.interdiff.txt1.91 KB
3041030-8.patch13.44 KB

#71, 2: Fixed.

The only part I'm missing that is mentioned in the IS is: "Rename it to a more meaningful name", which I think is worth doing.

The test class has been renamed from EfqTest to TaxonomyTermEfqTest:

+++ b/core/modules/taxonomy/tests/src/Kernel/TaxonomyTermEfqTest.php
@@ -1,47 +1,53 @@
-class EfqTest extends TaxonomyTestBase {
+class TaxonomyTermEfqTest extends KernelTestBase {
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at ezCompany commented

The test class has been renamed from EfqTest to TaxonomyTermEfqTest

Ah ok, I was looking for hopefully getting rid of 'Efq', why not just say TaxonomyTermEntityFieldQueryTest. No need to wrack my brain thinking what 'Efq' would stand for.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
FileSize
3041030-10.interdiff.txt697 bytes
3041030-10.patch13.47 KB

Got it. In fact the "entity field query" is an antiquity from D7. It's simply "entity query" now.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented

Can we also change the name of the method testTaxonomyEfq() to testTaxonomyEntityQuery()?

core/modules/taxonomy/tests/src/Kernel/TaxonomyTermEntityQueryTest.php:31:

/**
 * Tests that a basic taxonomy entity query works.
 */
 public function testTaxonomyEfq() {
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

I'd suggest testTermEntityQuery then, possibly for the class name also leave out the Taxonomy.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
FileSize
interdiff-10-13.txt2.73 KB
3041030-13.patch13.02 KB

1. I changed trigger_error() message. Drupal core deprecation policy.

The PHPdoc text and the trigger_error() message should follow the same format

According to #12:

2. I changed the class name from TaxonomyTermEntityQueryTest to TermEntityQueryTest.

3. I changed the name of the method testTaxonomyEfq() to testTermEntityQuery()

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at ezCompany commented
Status: Needs review » Reviewed & tested by the community

Thanks for all the feedback, this reads much better now.

Nice conversion, no loss of coverage that I can see.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3041030-13.patch, failed testing. View results

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at ezCompany commented
Title: Convert functional EfqTest to kernel test » Convert functional EfqTest to a kernel test and move TaxonomyTestTrait from tests/src/Functional to tests/src/Traits

New deprecation introduced due to #3042640: Convert TaxonomyFieldTidTest to a kernel test.

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs work » Needs review
FileSize
interdiff-13-17.txt648 bytes
3041030-17.patch13.65 KB
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs review » Reviewed & tested by the community

The patch has been RTBCed in #14. I'm doing it only for the change from #17.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 6874218 and pushed to 8.8.x. Thanks!

  • catch committed 6874218 on 8.8.x 
    Issue #3041030 by claudiu.cristea, Krzysztof Domański, Lendude, Berdir,...

Status: Fixed » Closed (fixed)

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

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

FYI: The change record for this was never published. I just did so.

I also figured out how to get my contrib tests that were trying to use this trait to work on 8.7.x and 8.8.x and above, so I added that wisdom to the CR:

https://www.drupal.org/node/3041703/revisions/view/11349728/11635421

Enjoy,
-Derek