Objective

  1. Rename DrupalUnitTestBase to KernelTestBase, because that's what it actually is (today).

  2. You have a functional kernel/service-container, no more, no less.

  3. In particular the "Unit" part is 100% misleading, because it is NOT about unit testing — if anything, it's "Integration".

  4. The naming was an explicit after-thought in the issue that introduced DrupalUnitTestBase.

Proposed solution

  1. Rename DrupalUnitTestBase to KernelTestBase.

  2. If necessary, retain an (empty) DrupalUnitTestBase extends KernelTestBase class as a BC shim.

Notes

  1. A new HttpKernelTestBase (DUTB + HttpKernel) is in the pipeline already. The proposed new name actively takes that into account.

    HttpKernelTestBase will provide the Request/Response architecture to tests, but without an external HTTP layer in between. It will enable very fast end-to-end request-response assertions, without the need to install a fully-fledged test site.

    The concept is used and natively supported by Symfony, too. (Although we're not able to use the native toolset, as we're missing the BrowserKit component, and that component duplicates functionality offered by Guzzle to some good extent to begin with...)

CommentFileSizeAuthor
#25 interdiff-2259275-14-23.txt645 bytesfilijonka
#5 2259275-5.patch21.1 KBfilijonka
#9 interdiff-2259275-5-9.txt3.42 KBfilijonka
#9 2259275-9.patch22.56 KBfilijonka
#13 interdiff-2259275-9-13.txt709 bytesfilijonka
#13 2259275-13.patch22.59 KBfilijonka
#14 test.kernel.14.patch42.76 KBsun
#14 interdiff.txt9.64 KBsun
#17 interdiff.txt75.43 KBxano
#17 drupal_2259275_17.patch137.89 KBxano
#23 interdiff-2259275-14-23.patch645 bytesfilijonka
#23 2259275-23.patch29.22 KBfilijonka

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Parent issue: » #1774388: Unit Tests Remastered™

Original issue.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +API clean-up, +Novice
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +API change, +Needs change record

I like the name but this is an API change, so tagging accordingly, this will affect a lot of modules that already ported tests to DrupalUnitTestBase. It's easy to fix, but we should still notify maintainers about it and I guess it will need a committer approval about the API change?

filijonka’s picture

filijonka commented
Assigned: Unassigned » filijonka
Status: Active » Needs review
StatusFileSize
new 2259275-5.patch21.1 KB

a first attempt to even see if I'm in the right direction. There are obvious some doc that needs to be changed but ignoring most of them for now. Cleaned up some things that didn't followed the coding standard.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

The coding style/standard changes are out of scope here, please remove them from the patch.

Instead, yes, we need to fix all references to DrupalUnitTestBase, and we additionally need to check whether the sloppy acronym DUTB might have slipped into a code comments somewhere.

We also need to add a @deprecated phpDoc tag to the old class, pointing to the new class name.

Aside from that, the patch appears to do exactly what's desired. FWIW, I wouldn't even mind if we'd ship with the old DUTB class. It would be great to remove it later (prior to beta/stable release or so), but as long as it doesn't block anything else, it would be fine to just leave it around.

filijonka’s picture

filijonka commented

well if removing whitespaces etc bothers you that much I'll add them back.

about leaving the DUTB class as is if I understood you correctly I don't think that is a good idea.
Simply because let us say that we make changes in the KernelTestBase class this won't be happening in the DUTB class simply cause it's functions will override the KTB class. If empty the KTB class will always be ruling.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Yes, sorry for not being clear — with "we can leave DUTB around" I meant exactly what this patch is already doing; i.e., turn the DrupalUnitTestBase class into an empty wrapper around KernelTestBase.

filijonka’s picture

filijonka commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2259275-5-9.txt3.42 KB
new 2259275-9.patch22.56 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

+1 to leaving DrupalUnitTestBase behind.

Minor nit

+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -26,443 +18,7 @@
+ * @deprecated since version 8.x-dev.Use \Drupal\simpletest\KerneltestBase

Missing space after 8.x-dev.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: -API change

Therefore not an API change

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Ok works for me, yeah I think the @deprecated doesn't follow the standard anyway, should be something like this:

+ * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
+ *   Use \Drupal\simpletest\KerneltestBase.
+ *
filijonka’s picture

filijonka commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2259275-9-13.txt709 bytes
new 2259275-13.patch22.59 KB

The doxygen for @deprecated says that the form should be @deprecated Description + saying what the description should say. nothing more about its structure so I kept the structure but made sure it was within 80chars of course.
Also corrected spelling of the KernelTestBase.

thanks for the quick feedback

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new test.kernel.14.patch42.76 KB
new interdiff.txt9.64 KB

Thanks! Final clean-ups:

  1. Renamed DrupalUnitTestBaseTest accordingly.
  2. Replaced all instances of DrupalUnitTestBase/DUTB with KernelTestBase.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -8,18 +8,10 @@
- *
- * @deprecated since version 8.x-dev, will be removed before Drupal 9.0.
- * Use \Drupal\simpletest\KernelTestBase
+ * @deprecated 8.0
+ *   Use \Drupal\simpletest\KernelTestBase
  */

As mentioned above, the previous format is what we're doing I thought? Search for @deprecated...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Similar to the @since phpDoc directive, the value of @deprecated specifies the version in which a resource has been deprecated.

Fundamentally, phpDoc directives support values, and (optionally) a separate description. Not to be intermixed. The value must be in a machine-readable format.

Flagging something as @deprecated 8.0 means that it has been deprecated in the 8.0 stable release. By design, and by definition of all available versioning standards, resources cannot be removed within stable releases. Therefore, @deprecated 8.0 logically implies that the resource will exist and will be available until the next major version.

Conversely, flagging something as @deprecated 8.x means that the resource can and probably will vanish prior to the next stable/major release.

All of this is really trivial logic and follows common sense. It's beyond me why we even started to add those invalid phpDoc directives to core. Plenty of my patches have added proper @deprecated directives to core already, following the simple logic outlined above.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
StatusFileSize
new interdiff.txt75.43 KB
new drupal_2259275_17.patch137.89 KB

Replaced all instances of DrupalUnitTestBase/DUTB with KernelTestBase.

It looks like a bunch of usages weren't converted?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I did not convert all tests/consumers, because I wasn't sure whether it would cause needless havoc for the rest of the queue... ('cos the old class will stay anyway for now) - don't have a preference myself, mostly depends on what core committers think.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Oh, duh. For some reason I thought your patch removed DUTB. Don't ask me why. Feel free to ignore my patch if that is easier.

filijonka’s picture

filijonka commented

I would say it's better to keep this as clean and small as possible and when that is done create (in new issue) a new patch dealing with next step. I wasn't even sure if we would do 1, and 2 that sun did in #14. At least not 2.
Oterwise we might risc ending up with a huge patch that is hard to comprehensive(?). divide and conquer.

@berdir @sun: about the @deprecated, I was probably not clear about this; I agree with you both that this is the way it should be written but for me the documentation on this doesn't say it like that. And most people doesn't seem to write it that way. I'll ask jhodgdon one day when I've time about it.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/simpletest/lib/Drupal/simpletest/DrupalUnitTestBase.php
@@ -7,462 +7,11 @@
+ * @deprecated 8.0

This still needs to be fixed.

As of this comment, there are NO comments with @deprecated N, where N is a number.

https://drupal.org/node/1354#deprecated explains the correct format, which here should be

 * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
 *   Use \Drupal\simpletest\KernelTestBase.
filijonka’s picture

filijonka commented

Well can we be sure that this will be removed before 8.0?

filijonka’s picture

filijonka commented
StatusFileSize
new interdiff-2259275-14-23.patch645 bytes
new 2259275-23.patch29.22 KB

based this on patch in #14

  * @see \Drupal\views\Tests\ViewTestBase
- * @see \Drupal\simpletest\DrupalUnitTestBase

Not sure about the above is correct simply cause ViewUnitTest is not extending or having any reference with ViewTestBase, but perhaps should have.

Also I wonder how come we can move around use statement etc but not remove code violating the coding standard? Tht isn't either in the scope for this issue.

The last submitted patch, 23: interdiff-2259275-14-23.patch, failed testing.

filijonka’s picture

filijonka commented
StatusFileSize
new interdiff-2259275-14-23.txt645 bytes

oh sorry my interdiff got patch extension. here it is as txt..

filijonka’s picture

filijonka commented
Assigned: filijonka » Unassigned
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Just existing change notices needs update:

1) primary https://drupal.org/node/1829160
2) mansions https://drupal.org/node/2235431 and https://drupal.org/node/2235431

Also there should be a follow-up to clean-up usage of old class

catch’s picture

catch
he/him
Primary language English
commented

Well can we be sure that this will be removed before 8.0?

We can't (although I'd be OK with a rename - this will break tests for contrib modules, but only the tests), but that's a better promise to break than saying it'll be removed later. Didn't review the patch yet.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

This needs a draft change record for the new class + deprecation, shouldn't be RTBC without one.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Reviewed & tested by the community

Created a follow-up for the conversion/removal (#2271273: Change all tests to extend from KernelTestBase instead of DrupalUnitTestBase + remove DrupalUnitTestBase) and created a change record: https://drupal.org/node/2271271.

The last submitted patch, 23: 2259275-23.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Hmmm this got committed as in 341b903 as part of #2228215: Remove module check in DrupalKernel

Fixed this with commit 2ae7acc and pushed to 8.x. Thanks!

  • Commit 2ae7acc on 8.x by alexpott: 
    Issue #2259275 by filijonka, sun, Xano: Rename "DrupalUnitTestBase" to...
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs change record

The CR is live. Thanks folks.

Status: Fixed » Closed (fixed)

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