Problem/Motivation

The test class in core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php uses a regular database query instead of using an entity query.

Proposed resolution

Replace the regular database query with an entity query.

Remaining tasks

TBD

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

TBD

CommentFileSizeAuthor
#13 3151953-13.patch1.36 KBLal_
#11 interdiff_9-11.txt565 bytesdurgeshs
#11 3151953-11.patch1.34 KBdurgeshs
#9 interdiff_6-9.txt897 bytesdurgeshs
#9 3151953-9.patch1.08 KBdurgeshs
#6 interdiff_3-6.txt551 bytesdurgeshs
#6 3151953-6.patch1.08 KBdurgeshs
#6 query.png48.92 KBdurgeshs
#6 tables.png78.14 KBdurgeshs
#6 output_tid_count.png29.49 KBdurgeshs
#3 3151953-3.patch1.09 KBsnehalgaikwad
#2 3151953-2.patch773 bytesdaffie

Comments

daffie created an issue. See original summary.

daffie’s picture

daffie commented
Status: Active » Needs work
StatusFileSize
new 3151953-2.patch773 bytes

There is something wrong with the test, because the query returns an empty array. See my added patch.

snehalgaikwad’s picture

snehalgaikwad commented
Status: Needs work » Needs review
StatusFileSize
new 3151953-3.patch1.09 KB

I replaced database query with an entity query but tests are failing. Previously database query was returning empty array so foreach wasn't getting executed but after replacing it with entity query now its is returning one value. Count in value is greater than 2 so test case is getting failed. Need to look into test case.

Status: Needs review » Needs work

The last submitted patch, 3: 3151953-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

durgeshs’s picture

durgeshs commented
Assigned: Unassigned » durgeshs
durgeshs’s picture

durgeshs commented
Assigned: durgeshs » Unassigned
Status: Needs work » Needs review
StatusFileSize
new output_tid_count.png29.49 KB
new tables.png78.14 KB
new query.png48.92 KB
new 3151953-6.patch1.08 KB
new interdiff_3-6.txt551 bytes

I have checked query and tables for above scenarios and found there are added translation when run testTranslationUI() method. And every time it is giving tid_count = 4;
Only local images are allowed.
Only local images are allowed.
Only local images are allowed.

Please check attached screenshots.

As per code, every time this code would be failed, either we can change code entityQueryAggregate with any other or change number of count under $this->assertTrue($tid['tid_count'] < 2, 'Term does not have translations.');

According to me, tid_count should be greater than 1, because in this scenario we are getting translated terms.

daffie’s picture

daffie commented
Status: Needs review » Needs work

@durgeshs: Thank you for working on this. Patch looks good, only I have a few remarks:

  1. +++ b/core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php
@@ -97,9 +97,14 @@ public function testTranslationUI() {
+    $tids =\Drupal::entityQueryAggregate('taxonomy_term')

    There needs to be a space between '=' and '\Drupal'. 

  2. +++ b/core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php
@@ -97,9 +97,14 @@ public function testTranslationUI() {
+    ->condition('vid', $this->bundle)

    This is not the correct condition. The field "vid" should NOT be equal to $this->bundle. 

  3. +++ b/core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php
@@ -97,9 +97,14 @@ public function testTranslationUI() {
+    ->aggregate('tid', 'COUNT')
+    ->condition('vid', $this->bundle)
+    ->groupBy('tid')
+    ->execute();

    These lines are not correctly indented. Each line needs 2 more spaces. 

  4. +++ b/core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php
@@ -97,9 +97,14 @@ public function testTranslationUI() {
+      $this->assertTrue($tid['tid_count'] > 1, 'Term does have translations.');

    Can we keep the part "< 2" and also keep the original message.

durgeshs’s picture

durgeshs commented
Assigned: Unassigned » durgeshs
durgeshs’s picture

durgeshs commented
StatusFileSize
new 3151953-9.patch1.08 KB
new interdiff_6-9.txt897 bytes

Hi @daffie: Thanks for suggestion to change coding standard.

I have updated all changes except of #4, we can keep "<2" and original message, because at this point term have translated value and we could not get count value lower than 2. If we keep #4, test case will be failed every time.

durgeshs’s picture

durgeshs commented
Assigned: durgeshs » Unassigned
Status: Needs work » Needs review
durgeshs’s picture

durgeshs commented
StatusFileSize
new 3151953-11.patch1.34 KB
new interdiff_9-11.txt565 bytes

Removed Unused use statement.

daffie’s picture

daffie commented
Status: Needs review » Needs work

With the following changes we change as little as possible and the test passes on my local machine.

  1. +++ b/core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php
@@ -97,9 +96,14 @@ public function testTranslationUI() {
+      ->condition('vid', 'tags')

    Can we change this line to: ->condition('vid', $this->bundle, '<>'). 

  2. +++ b/core/modules/taxonomy/tests/src/Functional/TermTranslationUITest.php
@@ -97,9 +96,14 @@ public function testTranslationUI() {
+      $this->assertTrue($tid['tid_count'] > 1, 'Term does have translations.');

    Can we change this line to: $this->assertTrue($tid['tid_count'] < 2, 'Term does not have translations.');.

Lal_’s picture

Lal_
He\Him
commented
Status: Needs work » Needs review
StatusFileSize
new 3151953-13.patch1.36 KB
daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

The query has been replaced by an entity query.
The code change looks good to me.
For me it is RTBC.

  • catch committed 6b587e6 on 9.1.x 
    Issue #3151953 by durgeshs, daffie, snehalgaikwad, Lal_: Replace the...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed 6b587e6 and pushed to 9.1.x. Thanks!

hardik_patel_12’s picture

hardik_patel_12 commented

Related issues are as follows.

#3151959: Replace the database query with an entity query in PathTaxonomyTermTest

#3151968: Replace the database query with an entity query in NodeTranslationUITest

#3151981: Replace the database query with an entity query in NodeRevisionsAllTest

#3151990: Replace the database queries with an entity queries in NodeRevisionPermissionsTest

Status: Fixed » Closed (fixed)

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