Convert web tests to browser tests for taxonomy module

Moved the non-views tests.

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?

Hi all!

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

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

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.

Use

cat ~/.gitconfig |grep rename
	renames = copies

to make patch smaller please

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.

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.

.....

\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.

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

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

@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.

Here we go with Views Tests as well.

Added followup issue for the out of the scope items. #2887134: Convert web tests to browser tests for taxonomy module Part -2

//Naveen

Here we go with fixes.

//Naveen

@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.

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?

@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

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

//Naveen

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

@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?

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

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

//Naveen

Will rerun locally

Fixed - no deprecation error now - thanks

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.

Committed as ccbd1e1 and pushed to 8.3.x.

Thanks, Lee!
Updated #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) and added #2887134: Convert web tests to browser tests for taxonomy module Part -2 to the list

Thank you @larowlan!

This added the new TaxonomyTestTrait in the functional test namespace but I was using it in a kernel test in token.module, a bit weird to include it there now from a kernel test. AFAIK we can't have classes outside of one of the defined test suite locations, we have to put it somewhere I guess, maybe we should have a place to pust test "API" things like this trait?