Convert VocabularyCrudTest test to kernel test

Here we go

oops

+++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
@@ -1,30 +1,59 @@
+    $this->installConfig([
+      'field',
+      'filter',
+      'system',
+      'field_test',
+      'taxonomy_crud',
+      'text',
+    ]);

I noticed that the tests still pass if we take out field and text from here, should we?

I noticed that the tests still pass if we take out field and text from here, should we?

we can remove the field, but don't have a strong opinion because I found that in the core there are mixed examples available.
The reasons why the tests are passing without field because field_test module has a dependency on the entity_test module which has a dependency on the field module.
Changes look good to me.

Thanks @naveenvalecha for the feedback. I did notice that we do need field and text on the list of modules to enable. Its only the configuration installation that seems not to be required for the test. Perhaps its being installed like you said via dependencies.

1. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -1,30 +1,59 @@
   +  public static $modules = [
   

   s/public/protected

2. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -1,30 +1,59 @@
   +    'field_test',
   

   This module is not used.

3. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -1,30 +1,59 @@
   +  protected $vocabulary;
   ...
        $this->vocabulary = $this->createVocabulary();
   

   This vocab is used only in 2 tests. The other tests are deleting the vocab prior running their tets logic. Let's remove it as class property and create it as local var only in the tests that are requiring it. Then we can drop also the vocabs removal from the other tests.

4. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -1,30 +1,59 @@
   +    $this->installConfig([
   +      'field',
   +      'filter',
   +      'system',
   +      'field_test',
   +      'taxonomy_crud',
   +      'text',
   +    ]);
   

   Re #10, #11, #12: It seems that we don't need any config to be installed. Drop the whole $this->installConfig().

5. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -67,7 +96,7 @@ public function testTaxonomyVocabularyDeleteWithTerms() {
   -    $this->assertEqual($this->vocabulary->label(), $original_vocabulary->label(), 'Vocabulary loaded successfully.');
   +    $this->assertEquals($this->vocabulary->label(), $original_vocabulary->label(), 'Vocabulary loaded successfully.');
   

   In simpletest, the assert custom message has described the success. Contrary, the PHPUnit tests are describing the failure. We have to either recreate all messages to describe the failure or to, simply, remove them. I would go with the later. We should do the same for all test occurrences.

6. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -67,7 +96,7 @@ public function testTaxonomyVocabularyDeleteWithTerms() {
   @@ -77,7 +106,7 @@ public function testTaxonomyVocabularyLoadStaticReset() {
   

   I don't understand what this test is testing. All these tests should be covered by generic config entities testing. I would remove the whole test.

7. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -77,7 +106,7 @@ public function testTaxonomyVocabularyLoadStaticReset() {
   @@ -107,34 +136,42 @@ public function testTaxonomyVocabularyLoadMultiple() {
   

   The method starts with an empty line.

8. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -107,34 +136,42 @@ public function testTaxonomyVocabularyLoadMultiple() {
        // Fetch the vocabularies with entity_load_multiple(), specifying IDs.
   

   entity_load_multiple() is deprecated and should be replaced in the comment.

9. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -107,34 +136,42 @@ public function testTaxonomyVocabularyLoadMultiple() {
        $this->assertIdentical($loaded_order, $expected_order);
   

   This method is deprecated.

10. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
    @@ -107,34 +136,42 @@ public function testTaxonomyVocabularyLoadMultiple() {
    +    $controller = $this->container->get('entity_type.manager')->getStorage('taxonomy_vocabulary');
    

    The naming: s/$controller/$storage

Re #13:
Thanks so much for the review!
1. Done
2.Nice, removed.
3. We actually use the vocabulary on testTaxonomyVocabularyDeleteWithTerms apparently, but I generally like this approach too, so I've moved it out of setUp().
4. Brilliant, removed.
5. Done.
6. I honestly don't know but I'd assume so, if they are not covered by generic entity testing, we should move it over there. Removing it here in any case for now.
7. Fixed.
8. Done.
9. Done.
10. Done.

Great. Thank you. I'm still seeing few issues.

1. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -32,43 +31,23 @@ class VocabularyCrudTest extends KernelTestBase {
   -    $this->assertEqual(0, $query->execute(), 'There are no terms remaining.');
   +    $this->assertEquals($query->execute(), 0);
   
   @@ -82,47 +61,20 @@ public function testTaxonomyVocabularyDeleteWithTerms() {
   -    $this->assertEqual(5, $query->execute(), 'There are 5 terms found.');
   +    $this->assertEquals($query->execute(), 5);
   ...
   -    $this->assertEqual(0, $query->execute(), 'All terms are deleted.');
   ...
   +    $this->assertEquals($query->execute(), 0);
   

   The order was correct before. In PHPUnit first come the expectation and that is 0 (or 5).

2. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -136,16 +88,16 @@ public function testTaxonomyVocabularyLoadMultiple() {
   -    $this->assertEquals($names[$vocabulary1->id()], $vocabulary1->id(), 'Vocabulary 1 name found.');
   +    $this->assertEquals($names[$vocabulary1->id()], $vocabulary1->id());
   

   It looks that $vocabulary1->id() is the expectation here. So it should be the first parameter.

3. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -199,7 +152,7 @@ public function testUninstallReinstall() {
        require_once $this->root . '/core/includes/install.inc';
   

   What is the reason for this file inclusion? I guess the test doesn't need it.

4. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -209,8 +162,8 @@ public function testUninstallReinstall() {
        // connected to this vocabulary name should have been removed when the
        // module was uninstalled. Creating a new field with the same name and
        // an instance of this field on the same bundle name should be successful.
   

   This docblock needs some love. The words "connected to this" could go one line up and the rest of lines could be reorganized to honour the coding standards rule from https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

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

5. +++ b/core/modules/taxonomy/tests/src/Kernel/VocabularyCrudTest.php
   @@ -209,8 +162,8 @@ public function testUninstallReinstall() {
   +    $vocabulary->enforceIsNew();
   +    $vocabulary->save();
   

   This can be chained as $vocabulary->enforceIsNew()->save();

Thanks again @claudiu.cristea for the review!

Re #15:

1. Good catch, fixed.
2. Yup, fixed.
3. Tested without it, and indeed it isn't necessary, removed it.
4. Yes, done.
5. Sure, done.

Some stats on the new kernel test: Time: 57.26 seconds, Memory: 6.00MB (on an Intel® Core™ i5-7200U CPU @ 2.50GHz × 4) - I would imagine the savings to be significant on this one, since it was 4 functional tests before, though I haven’t run them locally to measure the difference.

Thank you!

Unrelated DB server failure.

All looks good so setting back to RTBC! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).

The patch doesn't apply anymore.

Re-rolled #16 patch.

Thank you. Back to RTBC

Committed 202ad58 and pushed to 8.8.x. Thanks!