I don't exactly know what Core plans to do here, but EntityUnitTestBase extends the old KernelTestBase (\Drupal\simpletest\KernelTestBase), which is deprecated and scheduled to be removed by 8.2.0. Thus, in extension, EntityUnitTestBase should probably be considered deprecated as well – normally, it would not be a problem if that class just were changed to extend the new KernelTestBase (\Drupal\KernelTests\KernelTestBase) at some point, but in the case of tests this unfortunately would mean we'd need to move all our tests to different directories and namespaces as well, due to Drupal's separation between Simpletest and PHPUnit tests.

In Core, there is #2456477: Convert deprecated \Drupal\simpletest\KernelTestBase tests to KernelTestBaseNG for the migration, but it seems dormant for the moment. I've now asked there what contrib modules should do. Postponing this issue to wait for an answer there.

Remaining work:

\Drupal\search_api\Tests\Processor\ProcessorTestBase is still uses EntityUnitTestBase and it should use the new KernelTestBase.

Comments

drunken monkey created an issue. See original summary.

borisson_’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
61.33 KB

Changing this issue to NR, to enable the testbot to test this. There will be errors that come back from the tests:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The service "drupal.proxy_original_service.paramconverter.search_api" has a dependency on a non-existent service "user.shared_tempstore".

I can't figure out where those come from yet.

I took the opportunity of moving the files to make a couple of other changes to the tests:

  1. Removed "Unit" from the name of a couple of Kernel tests, because they are no unit tests.
  2. Changed all calls from assertEqual to assertEquals. Those calls are deprecated as well (there is a bc layer in AssertLegacyTrait, but we can just change our code). - that is a about 80% of this patch I think.
  3. Removed dependency on ExampleContentTrait in CliTest and CustomDataTypesTest
  4. Fixed a couple of 80cols violations for arrays.
  5. Added correct @var annotations in a couple of places.

The LanguageIntegrationUnitTest (renamed to LanguageIntegrationTest) had EntityLanguageTestBase as base class, that was 3 layers on top of the old kernel test base. I moved that too but I'm not sure if that'll just work. Still getting the error mentioned for that class as well.

Status: Needs review » Needs work

The last submitted patch, 2: move_tests_away_from-2642728-2.patch, failed testing.

The last submitted patch, 2: move_tests_away_from-2642728-2.patch, failed testing.

borisson_’s picture

There will be errors that come back from the tests:

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The service "drupal.proxy_original_service.paramconverter.search_api" has a dependency on a non-existent service "user.shared_tempstore".
I can't figure out where those come from yet.

I fixed this, we needed to install all required modules (incl. 'user', 'system' & 'entity_test').

The attached patch fixes:

  1. Tests now all run again
  2. A couple of extra 80 cols violations
  3. LanguageIntegrationTest now runs as well, I extracted the needed code from EntityLanguageTestBase. This

There should be 2 tests that still fail, "Drupal\search_api_db\Tests\BackendTest" and "Drupal\Tests\search_api\Kernel\CustomDataTypesTest". Both fail with the same error: "Error: Class name must be a valid object or a string". Figuring out how to fix that now.

Status: Needs review » Needs work

The last submitted patch, 5: move_tests_away_from-2642728-5.patch, failed testing.

The last submitted patch, 5: move_tests_away_from-2642728-5.patch, failed testing.

borisson_’s picture

The BackendTest was not discovered correctly. That should be fixed now. All tests should be green as well.

drunken monkey’s picture

Status: Needs review » Needs work

Thanks for your work on this!
However, the issue was postponed for a reason, I would really have preferred waiting for feedback in the Core issue.
However, now that the patch is almost done and already passing, I guess we might as well just go with it.

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The service "drupal.proxy_original_service.paramconverter.search_api" has a dependency on a non-existent service "user.shared_tempstore".

I can't figure out where those come from yet.

That's what I was talking about in #2642248: Depend explicitly on the User module? – it seems the parameter converter service will always be loaded, so now all our kernel tests require the user module as well.
Although it also might be possible to just insert a mock service, either for user.shared_tempstore or even directly for drupal.proxy_original_service.paramconverter.search_api itself.

Removed "Unit" from the name of a couple of Kernel tests, because they are no unit tests.

Well, they are in the sense that they are now PHP Unit tests. And I think it's good to have a reminder of that in the class name, to easily distinguish them.
This is especially true for LanguageIntegrationUnitTest – there already is a LanguageIntegrationTest in our module, so removing the Unit from the other one is pretty confusing. (Although that probably has the different problem that having both "integration" and "unit" in the class name is rather bizarre in and of itself. So maybe LanguageUnitTest instead, or LanguageKernelTest.)
Or maybe just change Unit to Kernel in the class names?

Also, changing assertEqual() calls to assertEquals() is of course correct and important, but please be aware that this has a different parameter order than we were mostly using in the old tests: assertEquals($expected, $actual, …)! That means you should also switch the parameters for most of the calls. (Luckily, should be possible with a regex, but probably still have to be reviewed individually.) Otherwise, the messages for test failures will be confusing.

Otherwise the patch looks great, thanks! (Although you can be lucky I'm not as strict as you are regarding unrelated changes. :P)

borisson_’s picture

Although it also might be possible to just insert a mock service, either for user.shared_tempstore or even directly for drupal.proxy_original_service.paramconverter.search_api itself.

It might be, but I don't think it slows our tests down to be explicit about this dependency. The fewer objects we have to mock, the better.

This is especially true for LanguageIntegrationUnitTest – there already is a LanguageIntegrationTest in our module, so removing the Unit from the other one is pretty confusing. (Although that probably has the different problem that having both "integration" and "unit" in the class name is rather bizarre in and of itself. So maybe LanguageUnitTest instead, or LanguageKernelTest.)

Changed to LanguageKernelTest as suggested, because this is probably the only one where the difference in name actually makes sense, for the other tests I disagree that this is important to have the namespace duplicated in the name and probably even counterproductive.

I also fixed the sequence of the arguments for all assertEquals calls in the classes we already touched, I did this by hand because it was already correct in some instances and my regex-fu was insufficient.

drunken monkey’s picture

Status: Needs review » Fixed

Looks all good, thanks again for your work!
Committed!

drunken monkey’s picture

Status: Fixed » Active

Just stumbled across it while glancing over #2638116: Clean up caching of Index class method results (especially fields): it seems we didn't port \Drupal\search_api\Tests\Processor\ProcessorTestBase here. Any reason, why, or did we just both overlook that?

borisson_’s picture

I figure we overlooked that, let's port that one as well.

borisson_’s picture

Issue summary: View changes
Issue tags: +Novice

Let's tag this as a novice issue, it's should fairly easy to resolve, and the previously committed patch is a good example of what to do.

borisson_’s picture

Not as easy as expected.

drunken monkey’s picture

Great job, thanks!
Due to #2638116: Clean up caching of Index class method results (especially fields), this required a re-roll, though.
Also, I refactored ContentAccessTest a bit to use a method for creating a user. Seems a bit cleaner and avoids duplicating code.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

That looks great, and the testbot agrees. Woo!

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Great. Committed.
Thanks again!

Status: Fixed » Closed (fixed)

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