As we convert procedural code to OOP, it would be nice to have a mature testing framework to properly unit test the new classes we are writing. While our simpletest fork serves us well for integration/functional testing, it's not a mature unit testing framework.

As I demonstrated in #1875086-46: Improve DX of drupal_container()->get(), PHPUnit can easily be used with our code base, and its mocking framework can be hugely helpful in unit testing our classes.

I'd like to propose that we move to PHPUnit for unit tests only, and keep Simpletest for our existing DrupalWebTestCase/DrupalUnitTestCase tests.

Commits

#52 and #57 were both committed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

msonnabaum’s picture

Status: Active » Needs review
FileSize
2.85 MB

I worked on this at the code sprint in Sydney with matt2000, and the result is attached here.

This patch converts a few of our existing UnitTestBase tests to phpunit, which is runnable on the command line after cd-ing into "core" (it reads the phpunit.xml.dist in that dir) and we've also integrated these tests into the simpletest UI runner.

We chose to put them in their own "PHPUnit" category since they take less than a second to run and it's likely that you'll always want to run them anyways.

Also, I chose to vendor phpunit because it enables us to ship with everything you need to run tests, and it also makes it possible to retain support for using the simpletest UI runner.

Some known issues:

  • This issue only includes a few converted tests, because many of our existing unit tests still depend on procedural functions from bootstrap.inc, common.inc, etc. These can be converted once we move their dependencies to static methods we can autoload, but I didn't want to try to include that in this issue.

  • In composer.json, phpunit is pointing to a fork on my github account. This is because they have a version constraint for symfony yaml <2.2, and we're already using the 2.2 betas. This is totally temporary and we can point it back to the main repo after symfony 2.2 is officially released.

  • We only get messages from tests on failures, so they are blank in the simpletest UI for passes. This is because there currently isn't a phpunit output format that supplies this information. I think it's fine for now, but we should either add what we want to the JSON format and submit it as a PR to phpunit, or just create our own custom format if that's sufficiently pluggable.

  • We haven't removed the getInfo() method yet, since it's still needed for the simpletest UI. We could probably refactor it later to use something native to phpunit, but it doesn't feel urgent to me.

Although this patch isn't perfect, it's quite functional and should provide a good starting point to get in before feature freeze. The issues outlined above can be addressed in followups, which feel more than doable before code freeze.

msonnabaum’s picture

Also, I wanted to mention that I'm aware of #1801176: Deploy a PHPUnit system with a bottom-up approach. There are some good ideas in there, but I wanted to first try the simplest possible solution so we can get it into d8. Some of the work in that thread can possibly be added in as a followup.

matt2000’s picture

I shouldn't RTBC, because I wrote some of this, but I'm tempted. Consider this a big +1.

PHPUnit is a good idea. I had almost zero experience with it before working with msonnabaum this week, and it was very easy to grasp. My knowledge of Simpletest was fully applicable to the task, so I feel that those who only know Simpletest's Unit tests will lose nothing, and I could immediately see how PHUnit's mocking could solve problems that I'd struggled with in our current UnitTests.

msonnabaum’s picture

Also, we've been pushing to the phpunit branch here: http://drupal.org/sandbox/msonnabaum/1912434

Might be easier to review by adding that as a remote and diffing.

Status: Needs review » Needs work

The last submitted patch, d8-unittests2phpunit.patch, failed testing.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
2.87 MB

This version restores some missing files I deleted by mistake and it should also fix the failing test.

Crell’s picture

I'm +1 on the concept. Can you post a non-vendor patch that's reviewable, though? (I don't think we need to dreditor all of PHPUnit...)

msonnabaum’s picture

As Crell requested, here's a no-vendor-dir version.

Status: Needs review » Needs work

The last submitted patch, d8-unittests2phpunit-6.patch, failed testing.

msonnabaum’s picture

Status: Needs work » Needs review
FileSize
93.96 KB
2.87 MB
Xano’s picture

+++ b/core/tests/Drupal/Tests/Core/NestedArrayUnitTest.php
@@ -0,0 +1,148 @@
+  /**
+   * Form array to check.
+   */

Needs @var. (There are more issues like these)

+++ b/core/tests/Drupal/Tests/Core/NestedArrayUnitTest.php
@@ -0,0 +1,148 @@
+  public static function getInfo() {

Needs docblock (there are more issues like these)

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -0,0 +1,67 @@
+class UnitTestCase extends \PHPUnit_Framework_TestCase {

UnitTestBase, to replace the existing class?

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -0,0 +1,67 @@
+  public static function getInfo() {
+    return array(
+      'name' => 'test name',
+      'description' => 'test description',
+      'group' => 'somegroup',
+    );

Looks like debugging code. What about making the getInfo() method abstract?

We only get messages from tests on failures, so they are blank in the simpletest UI for passes. This is because there currently isn't a phpunit output format that supplies this information. I think it's fine for now, but we should either add what we want to the JSON format and submit it as a PR to phpunit, or just create our own custom format if that's sufficiently pluggable.

As a temporary solution, maybe we can add a pass assert that says there are no fails? This may prevent a few confused looks when people look at the test results and see there are none.

To sidetrack a little: I have written quite a few D7 tests, but hardly any for D8, and I am fairly unexperienced with Simpletest's internals. I do wonder how hard it would be to make web test cases extend \PHPUnit_Framework_TestCase as well. I believe most Simpletest asserts are easily mapped to PHPUnit's. Would it be too much work to move the actual web-test-related code to PHPUnit as well before Drupal 9? IIRC we already use (or are going to use) Guzzle as the internal browser, so what's left would be the code to set up a client site.

// Edit: I believe the naming standard for PHP unit testing elsewhere is \Drupal\Core\Foo, combined with \Drupal\Tests\Core\FooTest, and not ...\FooUnitTest. See the proposal from #1872006: Add parity test classes for lib/Core and lib/Component as well.

msonnabaum’s picture

The @var and docblock comments actaully don't relate to this patch, they are missing from the existing tests. I'd rather not use this issue to add a bunch of doc cleanups to existing code.

UnitTestBase, to replace the existing class?

I went with UnitTestCase to be consistent with PHPUnit_Framework_TestCase. If others feel strongly that it should be UnitTestBase I could live with that, it just seems confusing since I'm not actually removing Drupal\simpletest\UnitTestBase in this patch.

As a temporary solution, maybe we can add a pass assert that says there are no fails? This may prevent a few confused looks when people look at the test results and see there are none.

I'm not necessairly against that, I'd just rather avoid it for now and implement it properly in a followup.

Looks like debugging code. What about making the getInfo() method abstract?

You actually can't make static methods abstract. New patch explains this and just returns blank strings which should be more obvious.

Also, the webtest issue has been discussed elsewhere and I'd *really* like to keep that discussion out of this thread since there's no way we can do that for d8.

effulgentsia’s picture

Here's a smaller patch for dreditor review. It removes additional Composer generated files and uses renames = copies.

effulgentsia’s picture

+++ b/core/modules/simpletest/simpletest.module
@@ -157,6 +177,84 @@ function simpletest_run_tests($test_list, $reporter = 'drupal') {
+ * Executes the phpunit command. We do this via exec in a subshell so that the
+ * environment is isolated when running tests via the simpletest UI.

Are we sure it's okay to require all Simpletest UI users to have shell execution enabled? Does PHPUnit require this, or is it just a "nice to have" isolation (in which case, can we do it in a separate issue)?

+++ b/core/modules/simpletest/simpletest.module
@@ -397,10 +495,22 @@ function simpletest_classloader_register() {
+      drupal_classloader()->registerNamespace('Drupal\\' . $name . '\\Tests', DRUPAL_ROOT . '/' . dirname($file->uri) . '/tests');

Does PHPUnit require us to relocate unit tests from "lib" to "tests"? Or is that just to assist us in knowing what we've converted to PHPUnit?

+++ b/core/tests/Drupal/Tests/Core/Cache/BackendChainImplementationUnitTest.php
@@ -95,16 +95,16 @@ public function setUp() {
-    $this->assertNotIdentical(FALSE, $cached, 'Got key that is on all backends');
-    $this->assertIdentical(1231, $cached->data, 'Got the key from the backend 1');
+    $this->assertNotEquals(FALSE, $cached, 'Got key that is on all backends');
+    $this->assertEquals(1231, $cached->data, 'Got the key from the backend 1');

Does PHPUnit implement Equals/NotEquals as == or ===? If the former, it's a shame to have to relax so many of our assertions. Could we instead extend PHPUnit to support Identical/NotIdentical assertions?

+++ b/core/tests/Drupal/Tests/Core/Cache/GenericCacheBackendUnitTestBase.php
@@ -0,0 +1,541 @@
+abstract class GenericCacheBackendUnitTestCase extends UnitTestCase {

This abstract class doesn't appear to be used in the patch. What's the purpose of adding it in this issue?

msonnabaum’s picture

Are we sure it's okay to require all Simpletest UI users to have shell execution enabled? Does PHPUnit require this, or is it just a "nice to have" isolation (in which case, can we do it in a separate issue)?

I think it's reasonable to expect exec to be available in an environment where you're running tests. Even if that's not an option for whatever reason, you can still run them via the command line.

This isolation is essential. We could call the phpunit classes directly, but then we'd have tests executing in a polluted environment like our current unit tests do. phpunit also supports process isolation for individual tests, so I'd like to have that as an option in the future.

Does PHPUnit require us to relocate unit tests from "lib" to "tests"? Or is that just to assist us in knowing what we've converted to PHPUnit?

It doesn't, but I liked the separation. Also, I think it was a mistake that we moved tests into lib. Having lib/src and tests is the convention. I remember us following symfony's lead there, but I think we may have misinterpreted what they did when they moved their tests into their namespace. The end result for them was that each component was self-contained and had a Tests directory after the sub-tree split. That makes no sense for us.

Does PHPUnit implement Equals/NotEquals as == or ===? If the former, it's a shame to have to relax so many of our assertions. Could we instead extend PHPUnit to support Identical/NotIdentical assertions?

I somehow missed this when I looked for it last, but its assertSame/assertNotSame. New patch changes them.

This abstract class doesn't appear to be used in the patch. What's the purpose of adding it in this issue?

Good catch. I think I postponed the test that used it. Removed.

msonnabaum’s picture

Posted #1916936: Add vendored copy of phpunit to make this issue easier to review.

msonnabaum’s picture

And here's a new version with only the test changes now that we have the lib committed.

Anonymous’s picture

yay for this patch! looks good to me, a couple of small things.

+  $phpunit_tests = @$test_list['UnitTest'] ?: array();

@wat? can we do isset($test_list['UnitTest']) ? $test_list['UnitTest'] : array(); instead?

+  /**
+   * This method exists to support the simpletest UI runner. It should
+   * eventually be replaced with something native to phpunit.
+   *
+   * Also, this method is empty because you can't have an abstract static
+   * method. Sub-classes should always override it.
+   */
+  public static function getInfo() {
+    return array(
+      'name' => '',
+      'description' => '',
+      'group' => '',
+    );
+  }

if sub-classes should always override this, lets throw an exception with a useful error message?

msonnabaum’s picture

Disagree about the first bit, but the exception is a good idea. New patch does that.

Xano’s picture

Suppressing PHP errors with @ really says "I don't bloody care why it doesn't work and I'm not going to tell you why I don't care"

@ suppresses *any* error you might encounter and it is generally bad practice. isset() tells the reader that the value might not be set and that this is expected and normal behavior.

Sylvain Lecoy’s picture

Hi there, I actually provided a PoC of a simple Unit Test converted to PHPUnit in #1801176: Deploy a PHPUnit system with a bottom-up approach. The other part of the work was functional integration and db testing.

I stopped working on this a bit because of the lack of directions, but this is only a pause and if you need any help I am interested in the testing topic.

sun’s picture

msonnabaum’s picture

I am, but I think we need to start with the smallest possible implementation that we can get in quickly. I honestly have no interest in replacing simpletest completely, I just want to solve the problem we have currently where we're creating a ton of classes but we don't have a unit testing framework to test them properly.

msonnabaum’s picture

Here's a new version that makes beejeebus' other suggested change because I dont want to bike-shed over style.

sun’s picture

Hm. Integrating PHPUnit tests with Simpletest wasn't really on the plan. I'm not sure whether that is really desirable.

Regarding /lib vs. /tests, see my reply in #1872006-25: Add parity test classes for lib/Core and lib/Component. You're essentially running into that trap here, since the test runner needs to register all namespaces twice.

In general, our general stance was that adding PHPUnit support is not bound to feature freeze, since the testing framework is not a feature of Drupal. Therefore, all work on this topic was scheduled for post-feature-freeze.

I really do not feel comfortable with rushing something into core. This will have really major consequences down the line, and we have sufficient experience to know what those consequences will be, so IMHO we should carefully plan the architecture and implementation.

I don't want our PHPUnit integration to become the next simpletest-alike disaster that we'll duct-tape for the next 6 years. (That's not meant to be about the code you're proposing, but about the overall architectural design and implementation plan.)

msonnabaum’s picture

Hm. Integrating PHPUnit tests with Simpletest wasn't really on the plan. I'm not sure whether that is really desirable.

We're not integrating PHPUnit with Simpletest, we're just adding PHPUnit support to our existing UI runner. I don't want our ability to properly unit test to be hung up on an argument over whether we need a testing UI or not. That can always be done later.

Regarding /lib vs. /tests, see my reply in #1872006-25: [PHPUnit] Add parity test classes for lib/Core and lib/Component. You're essentially running into that trap here, since the test runner needs to register all namespaces twice.

There's no trap. The patch registers both and it works. This is a very very popular convention and what we were doing was a Drupalism.

In general, our general stance was that adding PHPUnit support is not bound to feature freeze, since the testing framework is not a feature of Drupal. Therefore, all work on this topic was scheduled for post-feature-freeze.

I'm concerned about doing all the work we've done without isolated unit tests. It's hiding dependencies and tight coupling from us and I don't think we should delay it any further.

I really do not feel comfortable with rushing something into core. This will have really major consequences down the line, and we have sufficient experience to know what those consequences will be, so IMHO we should carefully plan the architecture and implementation.

This patch is incredibly simple. It's a baby step towards a larger migration of our test framework. I'd like to solve an immediate problem rather than creating another huge bike-shed that will have to wait until D9 to be implemented. Let's iterate.

I don't want our PHPUnit integration to become the next simpletest-alike disaster that we'll duct-tape for the next 6 years. (That's not meant to be about the code you're proposing, but about the overall architectural design and implementation plan.)

With this patch, you can run PHPUnit from the command line with absolutely no drupal dependencies. That's a design goal I had from the start and I think it should always be retained. From there, we have to make compromises for the UI until we decide it can be removed (possibly never).

I think we have the same goals and I'd really like to work together to get this in and then continually improve it.

Status: Needs review » Needs work

The last submitted patch, d8-unittests2phpunit-24.patch, failed testing.

msonnabaum’s picture

Status: Needs work » Needs review

#24: d8-unittests2phpunit-24.patch queued for re-testing.

RobLoach’s picture

Although I'm not sure about adding PHPUnit directly into core, I do like the idea of starting to leverage it. This does seem like an interim solution before tests are completely translated and Simpletest use is alleviated.

Sylvain Lecoy’s picture

I glad that msonnabaum explained why the tests directory was not a problem and is a common convention which physically separates the production code from the test code. Moreover, the phpunit.xml.dist can find really easily testing classes with this convention.

In fact, in a testing context, registering twice as namespace existing is really not a concern at all. The only problem would be false positive generated by other module files in the tests directory, but these modules files would never have to be PSR-0 class so I guess it a false problem.

The second thing, when speaking about robustness, is more related to namespace declaration. I don't know the status of the idea of #1916410: DX: Allow programmer to specify a different namespace (a.k.a classes under lib/ folder) in .info files., but in case this is getting into core, we gonna have to support it in the tests as well.

sun’s picture

Issue tags: +PHPUnit, +Testing system

The /lib vs. /tests discussion belongs into a separate issue. This patch should not introduce separate /tests directories per extension/namespace-root and should stick with what we have.

Really, we can only go in one of both directions: Either we carefully discuss and plan the architecture, or we try to iterate in very small steps, without scope creep. There are sufficient other details we need to discuss for this initial step, which I haven't raised yet, but will get back to after this weekend.

msonnabaum’s picture

There are sufficient other details we need to discuss for this initial step, which I haven't raised yet, but will get back to after this weekend.

If you have technical arguments to make against the patch, I'd love to hear them now while I have time to work on it. Also happy to talk it through in IRC if that's easier.

effulgentsia’s picture

Category: feature » task
Priority: Normal » Major

In general, our general stance was that adding PHPUnit support is not bound to feature freeze, since the testing framework is not a feature of Drupal.

I agree with this, so recategorizing as a task.

I'm concerned about doing all the work we've done without isolated unit tests. It's hiding dependencies and tight coupling from us and I don't think we should delay it any further.

This is also a good point, so I'm raising this to major. Someone might decide at some point that it's unfair for this to count against our thresholds, but let's cross that bridge when we get to it. In the meantime, I think this properly keeps this on people's radar.

Crell’s picture

I'm not an expert on Simpletest or PHPUnit, but reading over the latest patch I didn't see anything that particularly jumped out at me as problematic.

I don't have a strong feeling on /lib/ vs. /tests/. If it makes it easier to do this patch, I'm fine with it. If it's going to turn into a bikeshed, though, I'm also OK with punting. I do like this issue in general, though, as it gets our foot in the door and lets us refine our unit tests without having to rip out the entire simpletest framework at once.

Berdir’s picture

+++ b/core/modules/breakpoint/tests/Drupal/breakpoint/Tests/BreakpointMediaQueryTest.phpundefined
@@ -116,7 +118,7 @@ public function testInvalidMediaQueries() {
-        $this->assertTrue(TRUE, format_string('%media_query is not valid.', array('%media_query' => $media_query)));
+        $this->assertTrue(TRUE, sprintf('%s is not valid.', $media_query));

Hm. We've used format_string() and not sprintf() in assertion messages for a reason. I guess we don't need to worry about XSS but variables with HTML could mess up the markup.

The removal of t() from these messages was blocked on having something like format_string(), so I don't think we can just remove it now.

+++ b/core/tests/Drupal/Tests/Core/NestedArrayUnitTest.phpundefined
@@ -51,26 +53,26 @@ function setUp() {
-    $this->assertEqual($value['#value'], 'Nested element', 'Nested element value found.');
+    $this->assertEquals($value['#value'], 'Nested element', 'Nested element value found.');

While assertEqualS() actually makes more sense I think, it is a bit unfortunate that you now have to remember and use different assertion methods for unit and web tests.

+++ b/core/tests/Drupal/Tests/UnitTestCase.phpundefined
@@ -0,0 +1,78 @@
+class UnitTestCase extends \PHPUnit_Framework_TestCase {

What is the reason for calling this UnitTestCase and not Base? Per our coding standards, base classes should have a Base suffix.

+++ b/core/tests/Drupal/Tests/UnitTestCase.phpundefined
@@ -0,0 +1,78 @@
+  protected function pass($message = NULL, $group = 'Other') {
+    return $this->assertTrue($message, $group);

Not sure why we provide a helper for pass() but not other assertion messages that are named differently?

chx’s picture

BadMethodCallException => \BadMethodCallException

chx’s picture

Oh and I absolutely love the core/tests directory can we move all the tests that are currently and erroneously hosted in system there? I know, laterz :)

msonnabaum’s picture

Hm. We've used format_string() and not sprintf() in assertion messages for a reason. I guess we don't need to worry about XSS but variables with HTML could mess up the markup.

This introduces an unnecessary dependency on unit tests. If you kept it in now, that test would fail. To make it pass, you'd need to include bootstrap.inc. Now you've contaminated your testing environment with new globally defined constants and functions. Granted, we do need to move these procedural functions to static methods eventually (that is my next follow-up to this issue), but in this case, format_string just seems unnecessary for a unit test.

The use of format_string in assertions is actually really annoying on the command line. We shouldn't assume HTML as the default output of our tests. A while back I posted #1477110: Fix php path detection in run-tests.sh that tried to add htmlspecialchars_decode() to runtests.sh just so the results would be readable. If we need to run it through htmlspecialchars() at some point, it should be done higher in the stack, where we know HTML is the target, not in our assertions.

While assertEqualS() actually makes more sense I think, it is a bit unfortunate that you now have to remember and use different assertion methods for unit and web tests.

I talked to Angie about this in Sydney and we agreed that as follow-up to this issue, we should add the necessary PHPUnit assertions to our simpletest classes as proxy methods to start the transition, so these would work in all tests.

What is the reason for calling this UnitTestCase and not Base? Per our coding standards, base classes should have a Base suffix.

See #12

Not sure why we provide a helper for pass() but not other assertion messages that are named differently?

I brought that over because one of the original tests I converted used it, but that test isn't included in this patch, so I've removed the pass method for now. I don't really like it anyhow.

New patch also includes chx's fix. Good catch.

msonnabaum’s picture

Ideally testbot should be running phpunit directly, but in the interest of not having testbot changes hold up this patch, I added basic support for run-tests.sh.

Confirmed with jthorson that testbot just reads the results of the simpletest table, so this patch should cover run-tests.sh in the same way that it works with simpletest UI.

If this works, we should see the phpunit tests in the testbot results.

Sylvain Lecoy’s picture

Can't we just remove the getInfo() method definition in UnitTestCase ?

Or at least the static keyword, firstly because it is read as ->getInfo() in simpletest.module whereas we should use late static binding (e.g. $class::getInfo()) [ref].

Secondly, this would allow the use of abstract on getInfo() method which is much cleaner than a runtime check and throwing an exception, which in fine result in the same error but checked at different time (one is at runtime, the other at interpretation time). I prefer the later for two reasons:

  1. Don't repeat yourself, and don't repeat what is already existing: throw new \BadMethodCallException("Sub-class must implement the getInfo method!") is a copy/paste of a feature already implemented at language level. @see PHP Fatal error: Class absclass contains 1 abstract method and must therefore be declared abstract or implement the remaining methods.
  2. If we really want to keep it, we should at least make it a \RuntimeException, not a \BadMethodCallException. It is good to remember that a BadMethodCallException should be triggered when a callback does not exist or arguments are missing [ref]. Errors which mimics language level errors should be a RuntimeException [ref].

Eventually, i'd like to see this drupalism method die in further patches and why not flag it now as deprecated (@see #1876842: [policy, no patch] Use @deprecated to indicate a deprecated method or function).

msonnabaum’s picture

It's static here because it's static in the other simpletest classes. I know it's awkward, but the conventions around getInfo are way out of scope for this issue. I'm only using it on UnitTestCase as a way to get things working quickly with our existing test runners.

I would prefer we left it as is as just replace it as soon as possible with something that can get the info from phpunit annotations or something. That can be done in a followup.

msonnabaum’s picture

The last version had a bug which made testbot only run a handfull of tests. This one should fix.

msonnabaum’s picture

Notice that the test results of the last patch include the new phpunit tests, so this can go in without *any* testbot changes.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yes, let's start using it!

cweagans’s picture

+1

This looks awesome!

webchick’s picture

Assigned: Unassigned » Dries

Well, that is pretty effing exciting. Thanks so much for addressing my feedback in Sydney.

Assigning to Dries since it's a new library, and the choice to introduce two different testing interfaces feels a lot like a product-level decision to me.

msonnabaum’s picture

Just a reminder to whoever commits to also credit matt2000.

matt2000’s picture

Thanks, msonnabaum.

For the record, I've been watching the thread, and I endorse all the changes that have been made to the original version of the patch.

+1

plach’s picture

Some "nice to fix" if this needs to be rerolled :)

+++ b/core/modules/simpletest/simpletest.module
@@ -157,6 +169,102 @@ function simpletest_run_tests($test_list, $reporter = 'drupal') {
+ *   The parsed results of phpunit's junit xml output, in the format of the ¶

Trailing whitespace.

+++ b/core/modules/simpletest/simpletest.module
@@ -157,6 +169,102 @@ function simpletest_run_tests($test_list, $reporter = 'drupal') {
+ * Executes the phpunit command. We do this via exec in a subshell so that the
+ * environment is isolated when running tests via the simpletest UI.

We don't use "we" in PHP docs, in fact the second sentence describes an implementation detail that perhaps would be better fit in an inline comment near exec().

+++ b/core/modules/simpletest/simpletest.module
@@ -568,3 +689,64 @@ function simpletest_library_info() {
+  // Load the PHPUnit configuration file, which tells us where to find the tests.

Comment exceeding column 80.

+++ b/core/modules/simpletest/simpletest.module
@@ -568,3 +689,64 @@ function simpletest_library_info() {
+ * Converts phpunit's junit xml output to an array of rows that can be inserted
+ * into the simpletest results table.

This should be one line (or have a one-line summary).

+++ b/core/tests/Drupal/Tests/UnitTestCase.php
@@ -0,0 +1,59 @@
+   * The string will always start with a letter. The letters may be upper or
+   * lower case. This method is better for restricted inputs that do not
+   * accept certain characters. For example, when testing input fields that
+   * require machine readable values (i.e. without spaces and non-standard

PHP docs are not wrapping correctly at column 80.

kim.pepper’s picture

Coding style cleanups as per #49.

Sylvain Lecoy’s picture

The BadMethodCallException still need to be changed to RuntimeException.

Not defining an inherited method is a language level problem and is very different than calling a method with bad arguments or calling a non existing callback.

kim.pepper’s picture

Changed BadMethodCallException to RuntimeException

Dries’s picture

I'm also in favor of this, and it doesn't sound like there is any technical arguments against it. @sun raised some concerns, but no one else (including myself) sees an issue with moving this a small step forward. Planning to commit this later today.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks all.

Sylvain Lecoy’s picture

Yay!

sun’s picture

Status: Fixed » Needs work

When trying to run a test now, you get this error:


    Warning: file_get_contents(sites\default\files\simpletest/phpunit-836.xml): failed to open stream: No such file or directory in simpletest_phpunit_xml_to_rows() (line 733 of core\modules\simpletest\simpletest.module).

    simpletest_phpunit_xml_to_rows('836', 'sites\default\files\simpletest/phpunit-836.xml')
    simpletest_run_phpunit_tests('836', Array)
    simpletest_run_tests(Array, 'drupal')
    simpletest_test_form_submit(Array, Array)
    call_user_func_array('simpletest_test_form_submit', Array)
    form_execute_handlers('submit', Array, Array)
    drupal_process_form('simpletest_test_form', Array, Array)
    drupal_build_form('simpletest_test_form', Array)
    drupal_get_form('simpletest_test_form')
    call_user_func_array('drupal_get_form', Array)
    Drupal\Core\EventSubscriber\{closure}()
    call_user_func_array(Object, Array)
    Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
    Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
    Drupal\Core\HttpKernel->handle(Object, 1, 1)
    Symfony\Component\HttpKernel\Kernel->handle(Object)
    drupal_handle_request()

    Exception: String could not be parsed as XML in SimpleXMLElement->__construct() (line 734 of \core\modules\simpletest\simpletest.module).

sun’s picture

Status: Needs work » Needs review
FileSize
861 bytes
msonnabaum’s picture

While that should probably be handled better, I believe your issue is windows related and it's already RTC: #1936010: phpUnit fails on Windows

sun’s picture

The error in #56 happens when you run a simpletest-test and no phpunit-test at all. PHPUnit should not be invoked in the first place.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

You're right that it probably shouldn't be run, but the error shouldn't occur when no tests are selected.

Either way, what this patch is doing is an improvement.

Anonymous’s picture

I was running into the same problem as sun, not on Windows but on a Mac. Applying his patch fixed it for me.

amateescu’s picture

There's also this issue for MAMP: #1936646: phpunit fails on MAMP

Anonymous’s picture

I tried the fix in #1936646: phpunit fails on MAMP and it worked for me. Thanks for pointing me to it.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Sylvain Lecoy’s picture

Do we have a battle plan to convert existing unit tests to PHP Unit ?

I have a PoC of FileStorageTest in #1872006: Add parity test classes for lib/Core and lib/Component which uses a mocked filesystem.

fgm’s picture

Status: Fixed » Needs work

Change notice, maybe ?

xjm’s picture

Title: Start using PHPUnit for unit tests » [Change notice] Start using PHPUnit for unit tests
Assigned: Dries » Unassigned
Priority: Major » Critical
Status: Needs work » Active
Issue tags: +Needs change record

Sounds like a good idea to me.

RobLoach’s picture

Issue tags: +PHPUnit Blocker

Tagged a few of these issues with "PHPUnit Blocker"

podarok’s picture

Issue tags: +CodeSprintUA

Looks like this is a top voted idea for upcoming CodeSprint UA 2013 http://drupal.ua/groups/drupal-head/drupal-codesprint-2013
so tagging

YesCT’s picture

no change notice yet...

Something also like http://drupal.org/node/325974 Drupal SimpleTest coding standards please.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

Sylvain Lecoy’s picture

Best practices on Unit Agile Testing: http://drupal.org/node/1814344

Feel free to mention it. :-)

Wim Leers’s picture

Without a change notice, all of the peculiarities of using PHPUnit-based tests are a massive productivity drain. I had to discover all these subtleties the hard way, because there is no change notice:

  • When you want to run tests again on the results page (by using the "run tests" button), that fails. Both with only PHPUnit tests and when also containing SimpleTest tests.
  • All PHPUnit-based tests are always listed in a "PHPUnit" group, despite having specified another group.
  • A different directory structure!
  • Using debug() is impossible. When not using the CLI test runner, the tests will just be stuck forever, with zero feedback. Thanks to ParisLiakos for pointing me to the CLI test runner.

I understand that all of these are due to fundamental differences between SimpleTest and PHPUnit. But if we want developers to use this, then at least rudimentary documentation needs to be available. Right now, to use PHPUnit, one is forced to figure out everything manually.

(And +10^10 for a CLI test runner that works well!)

tim.plunkett’s picture

Unlike SimpleTest, PHPUnit is actually maintained by someone else.

http://phpunit.de/manual/current/en/automating-tests.html

But @Wim Leers, as you have most recently stumbled on some problems, you are the most qualified person to write some helpful documentation. Please document your experiences.

msonnabaum’s picture

Well, the idea was to make it as seemless as possible for someone who is familiar with PHPUnit. Usually, you just find the directory with the phpunit.xml and run "phpunit" there and it works. This should be the case here.

THe only real peculiarity is that the getInfo method it required for the simepltest UI to work. I think it might be more worthwhile to just fix that than documented it, but it may not be a bad idea for now.

podarok’s picture

Issue tags: -CodeSprintUA

removing tag

Berdir’s picture

Status: Active » Needs review

The change notice isn't supposed to be or replace documentation. It's just there to say "Hey, it's here, look how awesome, now go and use it!" and show where to start when coming from 7.x/Simpletest.

Here's a start: https://drupal.org/node/2012184 :)

ParisLiakos’s picture

Title: [Change notice] Start using PHPUnit for unit tests » Start using PHPUnit for unit tests
Priority: Critical » Major
Status: Needs review » Fixed
Issue tags: -Needs change record

i think thats good enough.
did some changes
https://drupal.org/node/2012184/revisions/view/2716902/2717306
feel free to improve:)

ParisLiakos’s picture

also.
#1938068: Convert UnitTestBase to PHPUnit contains links to some conversions already happened, if someone wants to take a look

Status: Fixed » Closed (fixed)

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

chx’s picture

For posterity: I would like to apologize to the community for the RTBC on this issue. I now wish I didn't. I got blinded by the shiny. Sorry.

chx’s picture

Issue summary: View changes

notes commits (two)

jhedstrom’s picture