Scope:
All tests not deemed out-of-scope

Out of scope followup issue:#2887134: Convert rest of taxonomy web tests to browser tests
- TaxonomyTermIndentationTest this should be converted to a javascript test
- TermAutocompleteTest should be converted to a javascript test
- \Drupal\Tests\taxonomy\Functional\TermTest::testTermReorder should be split of and converted to a javascript test
- TermTranslationTest needs AssertBreadcrumbTrait to be converted
- Drupal\taxonomy\Tests\RssTest

Comments

michielnugter created an issue. See original summary.

michielnugter’s picture

michielnugter’s picture

Updated the scope for a small change in scope so we have less issues this one is postponed on.

Left views in as a dependency because of the huge amount of tests here.

If you feel like giving if a go, please don't wait and start with all the non-views tests :)

michielnugter’s picture

Component: phpunit » taxonomy.module
dawehner’s picture

Status: Postponed » Active
Jo Fitzgerald’s picture

Status: Active » Needs review
FileSize
10.37 KB

Moved the non-views tests.

claudiu.cristea’s picture

Quick review:

  1. +++ b/core/modules/taxonomy/src/Tests/TaxonomyTestTrait.php
    @@ -9,6 +9,9 @@
    + * @deprecated Scheduled for removal in Drupal 9.0.0.
    

    Again, we should use either "in Drupal 9.0.x" or "before Drupal 9.0.0". Also the "Use" word should go on the 1st line, even I know it's not nice, but we have this rule at https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

    Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over

  2. +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTestTrait.php
    @@ -0,0 +1,60 @@
    +  public function createVocabulary() {
    ...
    +  public function createTerm(Vocabulary $vocabulary, $values = []) {
    

    I wonder why these are public methods? I don't say they should not, I didn't investigate the matter. If it's not necessary let's keep the protected.

  3. +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTestTrait.php
    @@ -0,0 +1,60 @@
    +      'name' => $this->randomMachineName(),
    +      'description' => $this->randomMachineName(),
    ...
    +      'name' => $this->randomMachineName(),
    ...
    +        'value' => $this->randomMachineName(),
    

    Due to their nature, can be also randomString(), no?

Status: Needs review » Needs work

The last submitted patch, 7: 2870459-7.patch, failed testing.

zahord’s picture

Issue summary: View changes
FileSize
13.02 KB

Hi all!

I just created a patch of the taxonomy module for the BrowserTestBase.

andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 10: 2870459-10.patch, failed testing.

dawehner’s picture

  1. deleted file mode 100644
    index 838a9b0..0000000
    
    index 838a9b0..0000000
    --- a/core/modules/taxonomy/src/Tests/TaxonomyTestBase.php
    
    --- a/core/modules/taxonomy/src/Tests/TaxonomyTestBase.php
    +++ /dev/null
    
    +++ /dev/null
    @@ -1,39 +0,0 @@
    
    @@ -1,39 +0,0 @@
    -<?php
    -
    -namespace Drupal\taxonomy\Tests;
    -
    -use Drupal\field\Tests\EntityReference\EntityReferenceTestTrait;
    -use Drupal\simpletest\WebTestBase;
    -
    -/**
    - * Provides common helper methods for Taxonomy module tests.
    - *
    - * @deprecated Scheduled for removal in Drupal 9.0.0.
    - *   Use \Drupal\Tests\taxonomy\Functional\TaxonomyTestBase instead.
    - */
    -abstract class TaxonomyTestBase extends WebTestBase {
    

    Note: For backward compability reasons we should keep this file around

  2. +++ b/core/modules/taxonomy/tests/src/Functional/RssTest.php
    @@ -1,13 +1,13 @@
     /**
    - * Ensure that data added as terms appears in RSS feeds if "RSS Category" format
    - * is selected.
    + * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
    + * Use \Drupal\Tests\taxonomy\Functional\RssTest
      *
      * @group taxonomy
    
    +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyQueryAlterTest.php
    @@ -1,15 +1,16 @@
    - * Tests that appropriate query tags are added.
    + * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
    + * Use \Drupal\Tests\taxonomy\Functional\TaxonomyQueryAlterTest
      *
      * @group taxonomy
      */
    -class TaxonomyQueryAlterTest extends WebTestBase {
    +class TaxonomyQueryAlterTest extends BrowserTestBase {
    
    +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTermIndentationTest.php
    @@ -1,9 +1,10 @@
    -namespace Drupal\taxonomy\Tests;
    +namespace Drupal\Tests\taxonomy\Functional;
     
     /**
    - * Ensure that the term indentation works properly.
    + * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
    + * Use \Drupal\Tests\taxonomy\Functional\TaxonomyTermIndentationTest
      *
    
    +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTermPagerTest.php
    @@ -1,9 +1,10 @@
    +namespace Drupal\Tests\taxonomy\Functional;
     
     /**
    - * Ensures that the term pager works properly.
    + * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
    + * Use \Drupal\Tests\taxonomy\Functional\TaxonomyTermPagerTest
    
    +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTestBase.php
    @@ -1,15 +1,17 @@
      * Provides common helper methods for Taxonomy module tests.
    + *
    + * @deprecated Scheduled for removal in Drupal 9.0.0.
    + *   Use \Drupal\Tests\taxonomy\Functional\TaxonomyTestBase instead.
      */
    -abstract class TaxonomyTestBase extends BrowserTestBase {
    +abstract class TaxonomyTestBase extends WebTestBase {
    
    +++ b/core/modules/taxonomy/tests/src/Functional/TaxonomyTestTrait.php
    @@ -8,7 +8,8 @@
     /**
    - * Provides common helper methods for Taxonomy module tests.
    + * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
    + * Use \Drupal\Tests\taxonomy\Functional\TaxonomyTestTrait
      */
    

    Note: We just add the @deprecated bits to baee classes, not individual test classes

zahord’s picture

FileSize
157.57 KB

Hi,

I did the changes to maintain the file TaxonomyTestBase.php in the old location and update the text with the @deprecated, additionally I updated to fix issues with the files TaxonomyGlossaryTest, PrepareUninstallTest and EntityReferenceFieldAttributesTest according the last test result.

zahord’s picture

Status: Needs work » Needs review
andypost’s picture

Use

cat ~/.gitconfig |grep rename
	renames = copies

to make patch smaller please

Status: Needs review » Needs work

The last submitted patch, 14: 2870459-14.patch, failed testing.

dawehner’s picture

I agree with @andypost. Otherwise the patch is basically impossible to review :) See https://www.drupal.org/documentation/git/configure for more information about this.

Lendude’s picture

Bit of a cleanup and some fixes but very much a work in progress.

\Drupal\taxonomy\Tests\TermAutocompleteTest should be out of scope for now, it needs to be converted to a javascript test.

Lendude’s picture

Status: Needs work » Needs review

.....

Status: Needs review » Needs work

The last submitted patch, 19: 2870459-19.patch, failed testing.
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Lendude’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
6.86 KB
10.66 KB

\Drupal\Tests\taxonomy\Functional\TermTest::testTermReorder and TaxonomyTermIndentationTest both use drupalpostform to set values on hidden fields, which Mink doesn't allow you to do (users can't do it, so mink won't do it). Since in both instances this is done to emulate javascript behaviour, they should be done in a javascript test.

TermTranslationTest needs AssertBreadcrumbTrait to be converted first

VocabularyUiTest uses assertNoTitle which doesn't exist on BrowserTestBase, but .... the assertNoTitle comes right after an assertTitle, so the assertNoTitle is completely redundant, it will never fail if the previous asserting passes (and in PHPUnit will never be run if the previous assertion failed). So I think we can just take it out, it doesn't do anything.

All Views tests still need to be done, but lets see if this is green.

dawehner’s picture

VocabularyUiTest uses assertNoTitle which doesn't exist on BrowserTestBase, but .... the assertNoTitle comes right after an assertTitle, so the assertNoTitle is completely redundant, it will never fail if the previous asserting passes (and in PHPUnit will never be run if the previous assertion failed). So I think we can just take it out, it doesn't do anything.

+1

Status: Needs review » Needs work

The last submitted patch, 22: 2870459-22.patch, failed testing.
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
15.1 KB
1.12 KB

Fixing the failures of #22.

Created followup issue for the out-of-scope items of this issue #2887102: Convert web tests to browser tests for taxonomy module Part-2

//Naveen

Lendude’s picture

Status: Needs review » Needs work

@naveenvalecha try adding --find-copies to your git diff, it makes the patches smaller when deprecating and copying the traits.

Looks good so far, back to needs work for the Views tests.

naveenvalecha’s picture

Here we go with Views Tests as well.

naveenvalecha’s picture

Issue summary: View changes

Added followup issue for the out of the scope items. #2887134: Convert rest of taxonomy web tests to browser tests

//Naveen

Status: Needs review » Needs work

The last submitted patch, 27: 2870459-27.patch, failed testing. View results

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
26.02 KB
572 bytes

Here we go with fixes.

//Naveen

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

@naveenvalecha thanks so much!

The patch looks good.

Checked the files that are left: 5 tests left and these are all marked for a follow up.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

I ran these locally using just the bare phpunit -c core --filter=taxonomy

Got one deprecation notice

Drupal\taxonomy\Tests\TaxonomyTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\taxonomy\Functional\TaxonomyTestBase: 1x
    1x in FunctionalTestSuite::suite from Drupal\Tests\TestSuites

Is that expected given the planned followups?

I think it indicates that something in our phpunit based tests is using the simpletest - which might point to something we missed?

Lendude’s picture

Status: Needs review » Needs work

@larowlan++

There are two tests in the RDF module that are in the PHPUnit namespace but extend WebTestBase version of TaxonomyTestBase.
\Drupal\Tests\rdf\Functional\EntityReferenceFieldAttributesTest
\Drupal\Tests\rdf\Functional\TaxonomyAttributesTest

Those we need to fix and extend from the BrowserTestBase base class because it leads to weirdness like you describe. @Mile23 figured out what was happening here: #2876145: ConfigTranslationViewListUiTest is a WTB test in the Functional namespace

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
89.26 KB
1.16 KB

#32,
Good catch :) +1
Addressed #33 in the same patch.

//Naveen

Status: Needs review » Needs work

The last submitted patch, 34: 2870459-34.patch, failed testing. View results

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
27.18 KB

Here's the patch. local branch needs rebasing before patch generation.

Lendude’s picture

@naveenvalecha thanks for the addition. This should fix it.

I can't seem to reproduce the fail in #32 in my setup. @larowlan could you check again with the patch in #36 or @naveenvalecha did you manage to reproduce #32 and seen it pass with #36?

naveenvalecha’s picture

I can't able to reproduce #32 on my local setup, but wouldn't able to reproduce it on my local.

$ ../vendor/bin/phpunit modules/taxonomy/tests/src
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing modules/taxonomy/tests/src
...............................................................  63 / 101 ( 62%)
......................................

Time: 20.77 minutes, Memory: 14.00MB

OK (101 tests, 2173 assertions)

//Naveen

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community

Setting it to RTBC so that we could get the review from @larowlan on #32

//Naveen

larowlan’s picture

Will rerun locally

larowlan’s picture

Fixed - no deprecation error now - thanks

larowlan’s picture

larowlan’s picture

Version: 8.4.x-dev » 8.3.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Ran the tests again locally, and triggered full bot run against 8.3 to evaluate backport.

For posterity, I was using ./drupal/vendor/bin/phpunit -c ./drupal/core --filter=taxonomy, not ../vendor/bin/phpunit modules/taxonomy/tests/src, guessing that is why you weren't able to reproduce - that matches 103 tests, not 101.

Committed as c4b5c0d and pushed to 8.4.x.

  • larowlan committed c4b5c0d on 8.4.x
    Issue #2870459 by naveenvalecha, Lendude, zahord, Jo Fitzgerald,...
larowlan’s picture

Committed as ccbd1e1 and pushed to 8.3.x.

  • larowlan committed ccbd1e1 on 8.3.x
    Issue #2870459 by naveenvalecha, Lendude, zahord, Jo Fitzgerald,...
naveenvalecha’s picture

dawehner’s picture

Thank you @larowlan!