Fatal error: Class 'Drupal\field\Tests\FieldUnitTestBase' not found

Adding a test run against Drupal 8.0.x.

So this is the issue that requires us to make a separate version of Examples for Drupal 8.1.x :-)

The problem here is that the CI automatically uses the latest dev branch of Drupal to run the tests.

If we make this change then no one using any previous version of Drupal (including the current stable release, 8.0.4) will be able to run the tests.

There's currently no way to mark your patch or issue or even contrib project as belonging to a certain core branch, so until Drupal 8.1.0 is released (or they fix it), we will have to run the test, let it fail, and then re-run it against the proper version of core.

Examples only tracks the current release of core, so we currently use the 8.0.x branch.

Since we don't have a way to specify semver for core or to specify which core branch belongs to which contrib release, I'm marking this issue as postponed until 8.1.0 is released.

Opened an issue in the core queue to bring back the old FieldUnitTestBase class for now: #2692223: Bring back the simpletest-based FieldUnitTestBase until 8.2.x

Just an update: #2692223: Bring back the simpletest-based FieldUnitTestBase until 8.2.x successfully punted the problem forward until the 8.2.x dev cycle begins in earnest.

Hopefully by then we'll be able to specify a core version for tests, but until then, this issue is postponed until the 8.2.0 release.

See also the status of this issue: #2456477: Convert deprecated \Drupal\simpletest\KernelTestBase tests to KernelTestBaseNG

Just an update: #2692223: Bring back the simpletest-based FieldUnitTestBase until 8.2.x successfully punted the problem forward until the 8.2.x dev cycle begins in earnest.

As I wrote already in a testbot issue, while there are other problems with not being able to control the core version, this specific case is resolved.

Once 8.1.0 is released, which is in 6 days, 8.0.x is unsupported. That means there is no reason to keep tests compatible with that version and it's perfectly fine for a module like this to update it's test so that they work with 8.1.x and 8.2.x.

Berdir: Examples is targeted to the latest stable release, which will at that point be 8.1.0, and has the overlap of both testing classes.

However, unless there's been some change I don't know about, the testbot will still run against the latest dev branch by default, which is 8.2.x, which will not have the deprecated classes.

So basically it doesn't matter. We'll still be inconvenienced for having the temerity to write tests. We'll see a failing patch, I'll have to restart it and explain to contributors that the testbot kind of sucks, and then life goes on.

Since the goal of Examples is to provide code for the latest stable release, we'll hold off on our deprecation until we get closer to 8.2.0. That way core can move the goalposts all it wants until that time and we'll still be playing by the rules here... The older test base class is deprecated for removal in 8.2.0.

If I could target this contrib project for testing against the latest stable, then all the problems would go away.

Once 8.1.0 is released, which is in 6 days, 8.0.x is unsupported. That means there is no reason to keep tests compatible with that version and it's perfectly fine for a module like this to update it's test so that they work with 8.1.x and 8.2.x.

@berdir -- the problem here is specific to 8.2.x. I've just verified that SimpleTest likes us fine for 8.1.x.

Is it really reasonable to require Contrib to test against the branch in development as a requirement? People using contrib modules, including this one, use it with the current branch. Since in this case, we can't work with both.

The problem here looks to be that the Field Permission Example (which I ported to 8.x) subclasses Drupal\field\Tests\FieldUnitTestBase. I'm guessing that the Core folk have changed how you test the Field related code, and have pulled the old SimpleTest-based code. Did you folks really need to do that for 8.2?

Looks like my base class disappeared as part of #2684095: Convert field kernel tests to KernelTestBaseTNG, in commit daa52e9f1ac4cb. Alex hates my tests, I guess :-)

BTW: this KernelTestBaseTNG business probably needs a change record, since Contrib folk do look at Core test code to do their tests. If we can't rely on some of this stuff, it would be better if we knew.

@torenware: The old KernelTestBase was marked as @deprecated before the 8.0.0 core release: http://cgit.drupalcode.org/drupal/tree/core/modules/simpletest/src/Kerne...

Here's the change notice: https://www.drupal.org/node/2489956

It's the only deprecation in core that breaks an API before the 9.0.0. release.

@Mile23 -- thanks. I'm modifying the code now to comply with that.

I've got a version that works on both 8.1 and 8.2.x.

The issue here is that kernel tests have moved from SimpleTest to PHPUnit. While this is in general a good thing, there are a lot of convenience methods for SimpleTest to deal with creating users and generating valid random strings, which do not yet exist for the new classes. I've discussed this with @larowlan, who thinks these methods need to get into core, but as of now, it hasn't happened in 8.2.x. The needed code is actually in my test class; it just needs to be turned into traits and checked into core (notice that due to other issues known to us, we can't just create the traits and add them to Examples; won't work).

I'd suggest that we check in the version of the test that folds the trait code in, and remove the cruft as soon as Core has the needed convenience methods. Until then, it's actually "best practice" to roll your own code for creating users and such. So the example will be useful with the extra stuff.

Most of what you are saying is not true :)

The old test was already a simpletest kernel test. It has more or less the same methods as phpunit kernel test. Specifically, the base class does not have createUser(), but that's exactly why it has the use UserCreationTrait line, which you removed.

If you re-add that, then you don't need to add it yourself and there is also no reason to add half a dozen methods that you don't actually need.

So, all that should be necessary, as of today (8.0.x is unsupported as of tomorrow), is to update the extends, move into the correct namespace and location.

An unrelated cleanup would be to move the array_unshift thing to protected $modules, not to construct like that.

And then it should work. If not, I'm happy to look into the test fails then.

An unrelated cleanup would be to move the array_unshift thing to protected $modules, not to construct like that.

@berdir --

What I'm going to write below is something of a rant, and I apologize to you in advance, since I'm aware you personally spend a lot of time on places like Drupal Answers helping people understand issues like this one. I do appreciate that, having benefitted from the time you put in there more than once.

But...

< BEGINS HERE THE RANT >

What is the alternative? I've read the notes in the source several times, and do not understand what's meant.

I'm overriding FieldKernelTestBase, which itself is a subclass of KernelTestBase.

In FieldKernelTestBase, we see declared:

  /**
   * Modules to enable.
   *
   * @var array
   */
  public static $modules = ['user', 'system', 'field', 'text', 'entity_test', 'field_test'];

But in KernelTestBase:

  /**
   * Modules to enable.
   *
   * Test classes extending this class, and any classes in the hierarchy up to
   * this class, may specify individual lists of modules to enable by setting
   * this property. The values of all properties in all classes in the class
   * hierarchy are merged.
   *
   * @see \Drupal\Tests\KernelTestBase::enableModules()
   * @see \Drupal\Tests\KernelTestBase::bootKernel()
   *
   * @var array
   */
  public static $modules = array();

This is actually unclear. If you are actually suggesting that in my class implementation I do:

  /**
   * Modules to enable.
   * 
   * WTF???? REALLY???
   *
   * @var array
   */
  public static $modules = ['FieldNoteItemTest'];

then you are suggesting something that developers from many (computer) language backgrounds find deeply counter intuitive: static inheritance post PHP 5.3 is a relatively new feature of the language, and you folks are doing something that looks rather strange. Most usages of an "override" replace the value of a member function or variable will alter the value of that variable. If you folks -- as I think you are -- are pulling distinct values up the inheritance tree -- you're doing something unexpected with the language. I looked through the code; it's actually unclear where exactly you are doing this: a code comment would help make this more clear. It's unusual enough that you probably need to modify the doc comment in KernelTestBase, to add something like

  /**
   * Modules to enable.
   *
   * Test classes extending this class, and any classes in the hierarchy up to
   * this class, may specify individual lists of modules to enable by setting
   * this property. The values of all properties in all classes in the class
   * hierarchy are merged.
   *
   * An example:
   *
   * @code
   *  public static $modules = ['module1', 'module2'];    
   * @endcode
   *
   * @see \Drupal\Tests\KernelTestBase::enableModules()
   * @see \Drupal\Tests\KernelTestBase::bootKernel()
   *
   * @var array
   */
  public static $modules = array();

As for the test traits: this is also a source of confusion. I'm not using Drupal\simpletest\UserCreationTrait (and adding its member functions) because another developer recommended it. Both of us are not unreasonably confused here, since in the source for the trait we see

/**
 * Provides methods to create additional test users and switch the currently
 * logged in one.
 *
 * This trait is meant to be used only by test classes extending
 * \Drupal\simpletest\TestBase.
 */
trait UserCreationTrait {

As it turns out, there isn't really a dependency here, and yes, it looks like I could use it. But the documentation right now in the 8.1.x tree says otherwise.

I understand that a number of core engineers, yourself included, have spent a lot of time over the last release trying to improve the test classes. But this went in w/o much of the documentation needed to bring it to the attention to contrib and other developers, who are not unreasonably confused. And they are discovering this not because they saw something in the change list, but because, as happened here, their tests simply stopped working, and it wasn't clear how to fix it. It's certainly slowed down work on Example. For example.

Developers here want to do the right thing, but currently, using the core tool set to test contrib modules has been quite frustrating, and something of a time sink. This is certainly not your intention, or the intention of other developers like Alex who have put a lot of time into improving the test system. But it's what's been happening for the last year that I've been more aware of the 8.x dev cycle, nonetheless.

There are a number of issues in the core queue right now from developers who are hitting a wall with the test bot and with issues like these. I have a couple myself, including #2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits, which I've kind of given up on. Some of the other Example developers have a number of them too.

This is an issue that I'd be happy to discuss with you in New Orleans if you're there, over beer, or bourbon, even. But the test system is definitely a pain point for contrib developers right now.

< ENDETH HERE THE RANT >

I'm now using the "declare via static::$modules" method for my module, and am using \Drupal\simpletest\UserCreationTrait. Should work on both 8.1 and 8.2, but not 8.0.

Great, you should also set up git so it properly reports renames in patches, see https://www.drupal.org/documentation/git/configure. Will make it much easier to see and review the differences.

Great, you should also set up git so it properly reports renames in patches, see https://www.drupal.org/documentation/git/configure. Will make it much easier to see and review the differences.

You're referring to this addition to .gitconfig?

[diff]
  renames = copies

If so, I've enclosed a patch regenerated with the new setting in place.

Yes exactly that, much easier to review now.

1. +++ b/field_permission_example/tests/src/Kernel/FieldNoteItemTest.php
   @@ -2,16 +2,20 @@
     * @file
   - * Contains \Drupal\field_permission_example\tests\FieldNoteItemTest.
   + * Contains \Drupal\Tests\field_permission_example\Kernel\FieldNoteItemTest'.
     *
   - * This is based off the tests used in Core for field plugins.
   + * This is based off the tests used in Core for field plugins. As of 8.1, it's
   + * recommended that tests like these inherit from KernelTestBase, or as we
   + * do here, from a subclass of it.
   

   Core removed all @file definitions from class files and the recommendation now is to no longer use them. Instead of fixing, I'd just remmove it and move the additional documentation to the class.

2. +++ b/field_permission_example/tests/src/Kernel/FieldNoteItemTest.php
   @@ -19,23 +23,34 @@ use Drupal\simpletest\UserCreationTrait;
   +   * @code
   +   * ['user', 'system', 'field', 'text', 'entity_test', 'field_test']
   +   * @endcode
   +   * as added by FieldKernelTestBase.
   

   Personally, i think just adding the reference would be enough, a list like this might get of sync and would then confuse more than help.

I've removed the @file doc, and changed the wording of the comment on $modules that that suggested by @jhodgdon in #2709581: Better explain the $modules variable in kernel tests. Should suffice.

+++ b/field_permission_example/tests/src/Kernel/FieldNoteItemTest.php
@@ -21,6 +12,10 @@ use Drupal\simpletest\UserCreationTrait;
+ * This class is based off the tests used in Core for field plugins. As of
+ * 8.1, it's recommended that tests like these inherit from KernelTestBase,
+ * or as we do here, from a subclass of it.

There are two KernelTestBase classes (for now) and this doesn't say which one, that makes this comment not very helpful :)

Has to use the full namespace (all comment references should, especially when not using them in the file)

There are two KernelTestBase classes (for now) and this doesn't say which one, that makes this comment not very helpful :)

I believe there are two BrowserTestBase classes as well.

Why is this, BTW?

Not sure, I guess it was initially placed there and removed, it's just empty subclass of the new location. Since tests are excluded from BC, I think this could be removed in 8.2.x or some other minor version too.

I'm assuming that the relevant classes are under core/tests/src/Drupal/Tests. Yes to that?

You might want to take a look at #2709581: Better explain the $modules variable in kernel tests, which deals with the wording issue, as well.

OK, modified with full namespaced class name.

Big thanks to both of you guys for sticking with it.

Reviewing....

1. +++ b/field_permission_example/tests/src/Kernel/FieldNoteItemTest.php
   @@ -1,41 +1,46 @@
   +// These work with PHPUnit tests as well as Simpletests.
   

   "We can use UserCreationTrait in Kernel tests as well as with Simpletests."

2. +++ b/field_permission_example/tests/src/Kernel/FieldNoteItemTest.php
   @@ -1,41 +1,46 @@
     * Tests the new entity API for the email field type.
   

   A bit out of scope here, but this test doesn't have anything to do with the email field type.

3. +++ b/field_permission_example/tests/src/Kernel/FieldNoteItemTest.php
   @@ -1,41 +1,46 @@
   + * This class is based off the tests used in Core for field plugins. As of
   + * 8.1, it's recommended that tests like these inherit from
   + * \Drupal\KernelTests\KernelTestBase, or as we do here, from a subclass of it.
   

   We don't really need to explain all this here. We're just inheriting whatever FieldKernelTestBase gives us. Let's keep "This class is based off the tests used in Drupal core for field plugins," and go on to explain why: "...since it has some helper methods for dealing with fields."

4. +++ b/field_permission_example/tests/src/Kernel/FieldNoteItemTest.php
   @@ -1,41 +1,46 @@
     * @group field
   

   Should be @group field_permission_example

5. +++ b/field_permission_example/tests/src/Kernel/FieldNoteItemTest.php
   @@ -87,7 +92,7 @@ class FieldNoteItemTest extends FieldUnitTestBase {
   +    // Fetch a form display for a user. This may already
        // exist, so check as Core does.
        // @see https://api.drupal.org/api/drupal/core%21includes%21entity.inc/function/entity_get_form_display/8
   

   Wrap out to 80 characters.

@Mile23 -- welcome back, stranger.

Changes now included.

Hi @Mile23/@Torenware,

Tested the patch on my local instance. Test results : Pass
Command used for testing : php core/scripts/run-tests.sh --directory modules/examples/field_permission_example/
Drupal version : 8.1.0
PHP : 5..6.10
MySQL : 5.5.42

Please, let me know if some others tests and reviews, I need to do.

Test Results screenshot

Thanks, @navneet0693.

If you've reviewed the issue and think it's ready to be committed, you can switch an issue status to 'reviewed and tested by the community.' That helps maintainers figure out what to do next. :-)

Feel free to review any issue.

Hi @Mile23,

Thanks for guidance, once this patch is committed. The Drupal CI error on https://www.drupal.org/node/2661834#comment-11121309 can be resolved.

Thanks, @navneet0693.

A couple things:

You can put the issue number in a square bracket and number sign like [#number] and it will link to that. Add -commentnumber to link to a comment: [#number-comment]

Also, if you think they're related, you can open up the 'Issue summary & relationships' section at the bottom of the page and paste in the related issue number. This makes a connecting link in both issues.

Aaaaaaand thanks to @berdir and @torenware for kicking it in the butt. :-)