Problem/Motivation

As #1567500: [meta] Pave the way to replace the testing framework with PHPUnit and possibly rewrite Simpletest is underway we need to convert all Simpletest based unittest to PhpUnit tests.

Proposed resolution

Let's create patches for each directory from the list.

Make sure you use

git diff -M 8.x

to make the file move explicit which reduces the patch size and thus the reviewers tasks.

Remaining tasks

Documentation for PHPUnit's mocks and stubs: http://phpunit.de/manual/current/en/test-doubles.html

As an example for modules see breakpoint module core/modules/breakpoint/tests/Drupal/breakpoint/Tests/
For Core or Component we already have several tests core/tests/Drupal/Tests/Core|Component.

  • If a test is in the system module, and it tests a class not in system module, then it should be moved to core/tests/Drupal/Tests/ directory
  • If a class contains Unit in its name, remove it when converting to PHPUnit..there is no point keeping it.
  • If a class can be converted to use dataproviders it should

As mentioned in #1938068-11: Convert UnitTestBase to PHPUnit

Open Issues

core/modules/simpletest/lib/Drupal/simpletest/Tests/MissingDependentModuleUnitTest.php cannot be converted to phpunit since it actually tests Simpletest itself (which requires Drupal bootstrap, etc). Not sure if it deserves a separate issue, or if it should just be removed from this issue summary.

Fill in the documentation void of http://drupal.org/node/1814344 Agile Unit Testing section 'Unit Test Organization'

TODO

#1996868: Start converting image.inc to an Image component
#1938130: Convert User tests to PHPUnit
#2003800: Move drupal_check_memory_limit() and parse_size() functionality to components
#2008566: Convert PasswordHashingTest unit tests to phpunit
#2035133: Convert system module's Mail unit test to phpunit
#2042739: Convert system module's ControllerResolverTest to phpunit
#2051467: Expand and convert to phpunit tests for \Drupal\Component\Transliteration
#2130551: Convert system modules MimeTypeMatcherTest to phpunit
(And then 44 more tests from system module.)

In core / lib / Drupal / Component

Component might be a good place to aim for 100% coverage.
Really though we want to make issues to remove procedural code from classes,
and declare the dependencies (dependency injection), and then add phpunit tests in the issue to "Declare dependecies in *classWhatever*".
Fill in issue numbers if related to getting those tests in, or issue numbers for new issues created to convert tests or improve coverage. Let's try and do one issue per class.

In core / lib / Drupal / Core / Batch

Batch has 3 classes, but 2 are not in phpunit coverage output since code is not autoloaded as part of tests.

Done

#2003762: Convert system module's Uuid unit tests to phpunit
#1938184: Convert Drupal\Core\Utility\Color\ColorTest to PHPUnit
#1991078: Convert MachineNameControllerTest to PHPUnit
#1901670: Start using PHPUnit for unit tests
#1935970: Convert timer_* to a utility class and convert tests to phpunit
#1957302: Convert PathProcessorTest to PHPUnit
#2001218: Convert core/modules/views/lib/Drupal/views/Tests/PluginTypeListTest.php to phpunit
#2003342: Convert system module's database unit tests to phpunit
#2002190: Convert core/modules/search/lib/Drupal/search/Tests/SearchExpressionTest.php to phpunit
#2003568: Convert tags,attributes, diff and url validation unit tests to phpunit
#2006568: Convert filter_xss tests to unit tests
#2016299: Convert system module's JsonUnitTest to phpunit
#2042745: Convert system module's RouteTest to phpunit
#2002116: Convert core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php to phpunit
#2030173: Convert system module's ValidNumberStepUnitTest to phpunit

API changes

The UnitTest files are moved to ... tbd
====
See coder #1938066: Prepare for phpunit tests

Related issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

Issue summary: View changes

Fixed wrong node

clemens.tolboom’s picture

Issue summary: View changes

Added link to coder

clemens.tolboom’s picture

Issue summary: View changes

Added issue summary

clemens.tolboom’s picture

Already done

grep --recursive --files-with-matches --exclude-dir=core/vendor UnitTestCase core/*
core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.php
core/modules/simpletest/simpletest.module
core/modules/system/lib/Drupal/system/Tests/Cache/GenericCacheBackendUnitTestBase.php
core/scripts/run-tests.sh
core/tests/Drupal/Tests/Core/Cache/BackendChainImplementationUnitTest.php
core/tests/Drupal/Tests/Core/Cache/NullBackendTest.php
core/tests/Drupal/Tests/Core/NestedArrayUnitTest.php
core/tests/Drupal/Tests/UnitTestCase.php

We need to register these.

clemens.tolboom’s picture

Issue summary: View changes

Added current list

franskuipers’s picture

Issue summary: View changes

Updated issue summary.

clemens.tolboom’s picture

Issue summary: View changes

Added block.

clemens.tolboom’s picture

Issue summary: View changes

Added block

clemens.tolboom’s picture

Issue summary: View changes

Added already done Unittests

franskuipers’s picture

Issue tags: +Create issues for missing UnitTests add above

Commands to find files/directories with UnitTests
All files:

grep --recursive --files-with-matches --exclude-dir=core/vendor 'simpletest\\UnitTestBase' core/*

All directories:

grep --recursive --files-with-matches --exclude-dir=core/vendor 'simpletest\\UnitTestBase' core/* | xargs -I {} dirname {} | sort -u

Current directories with UnitTestBase classes:

core/modules/image/lib/Drupal/image/Tests
core/modules/search/lib/Drupal/search/Tests
core/modules/simpletest/lib/Drupal/simpletest
core/modules/simpletest/lib/Drupal/simpletest/Tests
core/modules/system/lib/Drupal/system/Tests/Ajax
core/modules/system/lib/Drupal/system/Tests/Batch
core/modules/system/lib/Drupal/system/Tests/Bootstrap
core/modules/system/lib/Drupal/system/Tests/Cache
core/modules/system/lib/Drupal/system/Tests/Common
core/modules/system/lib/Drupal/system/Tests/Database
core/modules/system/lib/Drupal/system/Tests/Datetime
core/modules/system/lib/Drupal/system/Tests/DrupalKernel
core/modules/system/lib/Drupal/system/Tests/Graph
core/modules/system/lib/Drupal/system/Tests/KeyValueStore
core/modules/system/lib/Drupal/system/Tests/Mail
core/modules/system/lib/Drupal/system/Tests/Menu
core/modules/system/lib/Drupal/system/Tests/PhpStorage
core/modules/system/lib/Drupal/system/Tests/Plugin
core/modules/system/lib/Drupal/system/Tests/Plugin/Discovery
core/modules/system/lib/Drupal/system/Tests/Routing
core/modules/system/lib/Drupal/system/Tests/System
core/modules/system/lib/Drupal/system/Tests/Theme
core/modules/system/lib/Drupal/system/Tests/Uuid
core/modules/update/lib/Drupal/update/Tests
core/modules/user/lib/Drupal/user/Tests
core/modules/views/lib/Drupal/views/Tests
franskuipers’s picture

Issue summary: View changes

Added User module.

clemens.tolboom’s picture

Issue summary: View changes

added change notice

franskuipers’s picture

Issue tags: -Create issues for missing UnitTests add above +SprintWeekend2013
franskuipers’s picture

Issue summary: View changes

Updated issue summary.

franskuipers’s picture

Issue summary: View changes

Updated remaining tasks

Eric_A’s picture

Issue tags: +PHPUnit

Tagging phpunit.

Eric_A’s picture

Eric_A’s picture

Issue summary: View changes

Added tests.

Eric_A’s picture

Issue summary: View changes

There is an issue for user already.

Eric_A’s picture

Issue summary: View changes

Separate Done from To do.

Eric_A’s picture

Eric_A’s picture

Issue summary: View changes

Grouped by module.

Eric_A’s picture

EDIT: removed simpletest framework classes from the lists.

I've added non-system module UnitTestBase test files to the issue summary. I haven't checked if these have issues already.
Perhaps the 44 system module tests should be in a separate meta issue.

core/modules/image/lib/Drupal/image/Tests/ImageDimensionsScaleUnitTest.php
core/modules/search/lib/Drupal/search/Tests/SearchExpressionInsertExtractTest.php
core/modules/simpletest/lib/Drupal/simpletest/Tests/MissingDependentModuleUnitTest.php
core/modules/update/lib/Drupal/update/Tests/UpdateCoreUnitTest.php
core/modules/views/lib/Drupal/views/Tests/PluginTypeListTest.php
Eric_A’s picture

Issue summary: View changes

Removed simpletest framework classes.

ParisLiakos’s picture

Issue summary: View changes

Add Unicode DUTB

ParisLiakos’s picture

Issue summary: View changes

add transliteration test

ParisLiakos’s picture

Issue summary: View changes

Update issues with the new image dimension one

clemens.tolboom’s picture

Updated the summary to make sure we get patches using git -M

jhedstrom’s picture

Working on some of these, I notice that tests are being moved from modules into core/tests/.... If they are left in place, phpunit doesn't find them because they aren't being loaded from what I can tell. Is this intentional? For instance, see #1935908: Convert Graph tests to phpunit.

For instance, with core/modules/views/lib/Drupal/views/Tests/PluginTypeListTest.php or core/modules/search/lib/Drupal/search/Tests/SearchExpressionInsertExtractTest.php</code, should these be moved to <code>core/tests/..., or should we update core/bootstrap.php/ phpunit.xml.dist to find these where they currently live?

ParisLiakos’s picture

hi! thanks for working on this!
make sure you tag them with phpunit as well!

And yes unit tests for stuff in core should move in core/tests/ and for modules module_name/tests/

jhedstrom’s picture

re: #7 msonnabaum cleared this up. For modules, tests get moved from lib/Drupal/foo/Tests to tests/Drupal/foo/Tests.

jhedstrom’s picture

jhedstrom’s picture

re: #8oops, crosspost there. Thanks for the info!

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

core/modules/simpletest/lib/Drupal/simpletest/Tests/MissingDependentModuleUnitTest.php cannot be converted to phpunit since it actually tests Simpletest itself (which requires Drupal bootstrap, etc). Not sure if it deserves a separate issue, or if it should just be removed from this issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

clemens.tolboom’s picture

Issue summary: View changes

Added comment from jhedstrom to the remaind tasks list.
Moved todo issues to top of tasks.

clemens.tolboom’s picture

(And then 44 more tests from system module.)
#1935908: Convert Graph tests to phpunit

What is this remark about?

I currently have no clue what to do to convert one of the open tasks.

  • Where is the documentation?
  • What is the destination directory?

Can or Should we improve the issue summary?

Eric_A’s picture

(And then 44 more tests from system module.)

When I added that remark all non-system module UnitTestBase classes were already listed in the summary (with or without a linked issue), but not the 40+ classes from system module. See #5.

clemens.tolboom’s picture

@Eric_A but should these be listed in the todo of the issue summary as a todo or do we need XREFs to the issues or just remove it from the summary? I dunno :p

ParisLiakos’s picture

As an example for modules (for the destination question) see breakpoint module core/modules/breakpoint/tests/Drupal/breakpoint/Tests/
For Core or Component we already have several tests.

  • If a test is in the system module, and it tests a class not in system module, then it should be moved to core/tests/Drupal/Tests/ directory
  • If a class contains Unit in its name, remove it when converting to PHPUnit..there is no point keeping it.
  • If a class can be converted to use dataproviders it should
ParisLiakos’s picture

Issue summary: View changes

Made git diff -M better visible.
Added XREF to Agile Unit Testing http://drupal.org/node/1814344 assuming that's the official documentation page.

clemens.tolboom’s picture

@Eric_A + @ParisLiakos it would be awesome to have an up to date issue summary ;)

clemens.tolboom’s picture

Issue summary: View changes

added phpunit docs link

dawehner’s picture

Issue summary: View changes

added another test

dawehner’s picture

Issue summary: View changes

moved to todo

ParisLiakos’s picture

Issue summary: View changes

Add some more info and remove link to DUTB

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

ParisLiakos’s picture

Issue summary: View changes

Update done issues

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

organizing.

YesCT’s picture

Issue summary: View changes

added heading

YesCT’s picture

Issue summary: View changes

added some more class detail.

YesCT’s picture

Issue summary: View changes

adding a section for batch, why? because alexpott explained stuff using batch as an example and I dont want to lose that knowledge. -y

YesCT’s picture

Issue summary: View changes

added a trial issue for Settings test coverage. need to check with experienced people to see what approach to take for creating issues and describing them.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

added @ to get an overview sense of what is available and who is doing what. @ shows who it is assigned to.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

Mile23’s picture

BTW: Working on this #2032697: Create PHPUnit example module if anyone wants to opine.

Also made this: http://drupalcoverage.gopagoda.com/

Far from perfect, but hopefully useful.

Mile23’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

YesCT’s picture

Issue summary: View changes

added issue about accuracy of code coverage.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

Issue summary: View changes
sun’s picture

Component: base system » simpletest.module
Issue tags: -SprintWeekend2013 +Testing system

Thanks a ton to everyone who is working on this herculean effort! :-)

It looks like we're missing quite a few tasks for remaining UnitTestBase classes though?

Class hierarchy of UnitTestBase

Our ultimate goal should be to remove UnitTestBase prior to the release of Drupal 8.0.0. All modules should use PHPUnit for unit testing. For Simpletest, only DrupalUnitTestBase will remain for API-level integration tests that have access to the kernel, service container, and extension system.

Mile23’s picture

Just wanted to point out that, thanks to expanded use of @covers and @coversDefaultClass, the previously exciting and lovely 100% coverage for \Drupal\Component is now 38% at best.

Which is good, I guess. :-)

sun’s picture

What's the current status of this effort? It looks like we're very close to completion?

Out of the remaining, quite a few should simply be converted from UnitTestBase to KernelTestBase, because the tests implicitly assume a preexisting "parent site" (test runner), which happens to allow them to futz with settings, services, and filesystem paths, even though they are supposed to operate in a completely empty unit test environment.

It would be great to get rid of UnitTestBase as soon as possible.

Mile23’s picture

sun’s picture

To clarify my question:

All of the remaining Child issues of this meta issue are about expanding test coverage of existing/converted phpunit tests. Are there issues to convert the remaining tests based on UnitTestBase that might not be linked to this parent/meta issue?

Berdir’s picture

Status: Active » Needs review
FileSize
16.96 KB

I think not.

The attached patch converts all classes that extend directly from UnitTestBase (except KernelTestBase itself) to extend from KernelTestBase). Most are working fine and can be cleaned up nicely, but some are failing, in some cases expected (the GetFilename test) in others not, did not investigate yet.

Status: Needs review » Needs work

The last submitted patch, 25: convert-to-kernel-tests-1938068-25.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
20.57 KB
9.48 KB

+1 — Fixed the remaining test failures + cleaned up.

sun’s picture

Created the final follow-up issue: #2324789: Remove UnitTestBase

jibran’s picture

+++ b/core/modules/system/src/Tests/Bootstrap/GetFilenameUnitTest.php
@@ -45,3 +59,30 @@ function testDrupalGetFilename() {
+class NullState implements StateInterface {

This class missing docs.

sun’s picture

Added a comment.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Nice cleanup Berdir and Sun.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

It's confusing to me that this patch is in the meta, which would imply that it is the final patch needed, yet KernelTestBase still extends UnitTestBase, and we still have #2324789: Remove UnitTestBase.

Perhaps this issue should be combined with that, properly removing UnitTestBase? Or this one needs to be temporarily renamed, or split out.

jibran’s picture

Thanks for the doc fix @sun. You going to reroll it anyway so minor nitpick.

+++ b/core/modules/system/src/Tests/Bootstrap/GetFilenameUnitTest.php
@@ -45,3 +59,33 @@ function testDrupalGetFilename() {
+  public function get($key, $default = NULL) {
...
+  public function getMultiple(array $keys) {
...
+  public function set($key, $value) {
...
+  public function setMultiple(array $data) {
...
+  public function delete($key) {
...
+  public function deleteMultiple(array $keys) {
...
+  public function resetCache() {

I know this is stub class in phpunit that is why I am asking. Is it a good idea to add inheritdoc here?

sun’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
21.91 KB

Re-rolled against HEAD. Interdiff wasn't possible due to conflicts.


@jibran: Didn't add those, because it's a temporary, test-only, no-op class that implements a single interface only, and no other code can use it. The PHPDoc on the class is sufficient.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: convert-to-kernel-tests-1938068-34.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
22.99 KB
2.94 KB
tim.plunkett’s picture

It's confusing to me that this patch is in the meta, which would imply that it is the final patch needed, yet KernelTestBase still extends UnitTestBase, and we still have #2324789: Remove UnitTestBase

Perhaps this issue should be combined with that, properly removing UnitTestBase? Or this one needs to be temporarily renamed, or split out.

This was skipped in the recent round of feedback.

sun’s picture

@tim.plunkett: The vast majority of work happened in sub-issues. The patch performs the last remaining changes to complete the effort.

Intentionally created #2324789: Remove UnitTestBase as a separate issue, because it's a generic test base class. We don't have to break contrib tests immediately. The base class can be removed separately. Its removal is not necessary to complete the goal of converting all tests.

tim.plunkett’s picture

Title: [Meta] Convert UnitTestBase to PHPUnit » Convert UnitTestBase to PHPUnit

The vast majority of work happened in sub-issues. The patch performs the last remaining changes to complete the effort.

Yes, and the sky is blue and the earth is round. Thanks for explaining.

dawehner’s picture

This patch looks really fine. I especially like the KV test changes, as they have a much higher readability.

  1. +++ b/core/modules/simpletest/src/KernelTestBase.php
    @@ -169,8 +169,12 @@ protected function setUp() {
    +    if (get_class($this->container->get('keyvalue')) === 'Drupal\Core\KeyValueStore\KeyValueMemoryFactory') {
    

    Is there a reason we don't use $this->container->get('keyvalue') instanceof ... ? This makes it for example easier to read.

  2. +++ b/core/modules/system/src/Tests/Bootstrap/GetFilenameUnitTest.php
    @@ -45,3 +59,33 @@ function testDrupalGetFilename() {
    +  public function set($key, $value) {
    +  }
    +
    +  public function setMultiple(array $data) {
    +  }
    +
    +  public function delete($key) {
    +  }
    +
    +  public function deleteMultiple(array $keys) {
    +  }
    

    Totally unrelated but I realized we could improve the DX here in general: #2328129: Make KeyValueStoreInterface and StateInterface chainable

sun’s picture

Addresses #40 + simplifies GetFilenameUnitTest.

GetFilenameUnitTest could be converted into a phpunit unit test, but we do not add/use phpunit tests for legacy procedural functions. Instead, phpunit tests are only added when things are converted into classes/services, which makes sense.

Therefore, this is the only test that will temporarily use KTB even though it doesn't need a kernel/container. But we should get rid of drupal_get_path() + drupal_get_filename() ASAP anyway.

sun’s picture

Would be great to move forward here.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/system/src/Tests/Database/ConnectionUnitTest.php
@@ -252,7 +258,13 @@ public function testConnectionSerialization() {
+        $this->assertEqual($actual, $expected, vsprintf('Unserialized Connection property %s value %s is equal to expected %s', array(

still don't like vsprintf but well, no actual problem here.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Can't find any issues in glancing through, and this has been RTBC long enough to give someone else a chance to raise them if there are any. :)

Committed and pushed to 8.x. Thanks!

  • webchick committed 50eb47e on 8.0.x
    Issue #1938068 by sun, Berdir: Convert UnitTestBase to PHPUnit.
    

Status: Fixed » Closed (fixed)

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