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
Comment | File | Size | Author |
---|---|---|---|
#57 | _unit.patch | 861 bytes | sun |
#52 | phpunit-unittests-1901670-52.patch | 40.82 KB | kim.pepper |
#52 | interdiff.txt | 535 bytes | kim.pepper |
#50 | phpunit-unittests-1901670-50.patch | 40.82 KB | kim.pepper |
#50 | interdiff.txt | 4.61 KB | kim.pepper |
Comments
Comment #1
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedAlso, 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.
Comment #3
matt2000 CreditAttribution: matt2000 commentedI 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.
Comment #4
msonnabaum CreditAttribution: msonnabaum commentedAlso, 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.
Comment #6
msonnabaum CreditAttribution: msonnabaum commentedThis version restores some missing files I deleted by mistake and it should also fix the failing test.
Comment #7
Crell CreditAttribution: Crell commentedI'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...)
Comment #8
msonnabaum CreditAttribution: msonnabaum commentedAs Crell requested, here's a no-vendor-dir version.
Comment #10
msonnabaum CreditAttribution: msonnabaum commentedComment #11
XanoNeeds @var. (There are more issues like these)
Needs docblock (there are more issues like these)
UnitTestBase, to replace the existing class?
Looks like debugging code. What about making the getInfo() method abstract?
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.
Comment #12
msonnabaum CreditAttribution: msonnabaum commentedThe @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.
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.
I'm not necessairly against that, I'd just rather avoid it for now and implement it properly in a followup.
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.
Comment #13
effulgentsia CreditAttribution: effulgentsia commentedHere's a smaller patch for dreditor review. It removes additional Composer generated files and uses renames = copies.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedAre 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)?
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?
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?This abstract class doesn't appear to be used in the patch. What's the purpose of adding it in this issue?
Comment #15
msonnabaum CreditAttribution: msonnabaum commentedI 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.
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.
I somehow missed this when I looked for it last, but its assertSame/assertNotSame. New patch changes them.
Good catch. I think I postponed the test that used it. Removed.
Comment #16
msonnabaum CreditAttribution: msonnabaum commentedPosted #1916936: Add vendored copy of phpunit to make this issue easier to review.
Comment #17
msonnabaum CreditAttribution: msonnabaum commentedAnd here's a new version with only the test changes now that we have the lib committed.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedyay for this patch! looks good to me, a couple of small things.
@wat? can we do
isset($test_list['UnitTest']) ? $test_list['UnitTest'] : array();
instead?if sub-classes should always override this, lets throw an exception with a useful error message?
Comment #19
msonnabaum CreditAttribution: msonnabaum commentedDisagree about the first bit, but the exception is a good idea. New patch does that.
Comment #20
Xano@ 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.
Comment #21
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedHi 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.
Comment #22
sunAre you aware of the larger battle plan? #1567500: [meta] Pave the way to replace the testing framework with PHPUnit and possibly rewrite Simpletest
Comment #23
msonnabaum CreditAttribution: msonnabaum commentedI 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.
Comment #24
msonnabaum CreditAttribution: msonnabaum commentedHere's a new version that makes beejeebus' other suggested change because I dont want to bike-shed over style.
Comment #25
sunHm. 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.)
Comment #26
msonnabaum CreditAttribution: msonnabaum commentedWe'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.
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.
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 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.
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.
Comment #28
msonnabaum CreditAttribution: msonnabaum commented#24: d8-unittests2phpunit-24.patch queued for re-testing.
Comment #29
RobLoachAlthough 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.
Comment #30
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI 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.
Comment #31
sunThe /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.
Comment #32
msonnabaum CreditAttribution: msonnabaum commentedIf 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.
Comment #33
effulgentsia CreditAttribution: effulgentsia commentedI agree with this, so recategorizing as a task.
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.
Comment #34
Crell CreditAttribution: Crell commentedI'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.
Comment #35
BerdirHm. 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.
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.
What is the reason for calling this UnitTestCase and not Base? Per our coding standards, base classes should have a Base suffix.
Not sure why we provide a helper for pass() but not other assertion messages that are named differently?
Comment #36
chx CreditAttribution: chx commentedBadMethodCallException => \BadMethodCallException
Comment #37
chx CreditAttribution: chx commentedOh 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 :)
Comment #38
msonnabaum CreditAttribution: msonnabaum commentedThis 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.
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.
See #12
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.
Comment #39
msonnabaum CreditAttribution: msonnabaum commentedIdeally 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.
Comment #40
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedCan'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:
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).
Comment #41
msonnabaum CreditAttribution: msonnabaum commentedIt'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.
Comment #42
msonnabaum CreditAttribution: msonnabaum commentedThe last version had a bug which made testbot only run a handfull of tests. This one should fix.
Comment #43
msonnabaum CreditAttribution: msonnabaum commentedNotice that the test results of the last patch include the new phpunit tests, so this can go in without *any* testbot changes.
Comment #44
chx CreditAttribution: chx commentedYes, let's start using it!
Comment #45
cweagans+1
This looks awesome!
Comment #46
webchickWell, 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.
Comment #47
msonnabaum CreditAttribution: msonnabaum commentedJust a reminder to whoever commits to also credit matt2000.
Comment #48
matt2000 CreditAttribution: matt2000 commentedThanks, 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
Comment #49
plachSome "nice to fix" if this needs to be rerolled :)
Trailing whitespace.
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()
.Comment exceeding column 80.
This should be one line (or have a one-line summary).
PHP docs are not wrapping correctly at column 80.
Comment #50
kim.pepperCoding style cleanups as per #49.
Comment #51
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedThe
BadMethodCallException
still need to be changed toRuntimeException
.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.
Comment #52
kim.pepperChanged
BadMethodCallException
toRuntimeException
Comment #53
Dries CreditAttribution: Dries commentedI'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.
Comment #54
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks all.
Comment #55
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedYay!
Comment #56
sunWhen trying to run a test now, you get this error:
Comment #57
sunComment #58
msonnabaum CreditAttribution: msonnabaum commentedWhile that should probably be handled better, I believe your issue is windows related and it's already RTC: #1936010: phpUnit fails on Windows
Comment #59
sunThe 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.
Comment #60
msonnabaum CreditAttribution: msonnabaum commentedYou'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.
Comment #61
Anonymous (not verified) CreditAttribution: Anonymous commentedI was running into the same problem as sun, not on Windows but on a Mac. Applying his patch fixed it for me.
Comment #62
amateescu CreditAttribution: amateescu commentedThere's also this issue for MAMP: #1936646: phpunit fails on MAMP
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedI tried the fix in #1936646: phpunit fails on MAMP and it worked for me. Thanks for pointing me to it.
Comment #64
webchickCommitted and pushed to 8.x. Thanks!
Comment #65
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedDo 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.
Comment #66
fgmChange notice, maybe ?
Comment #67
xjmSounds like a good idea to me.
Comment #68
RobLoachTagged a few of these issues with "PHPUnit Blocker"
Comment #69
podarokLooks like this is a top voted idea for upcoming CodeSprint UA 2013 http://drupal.ua/groups/drupal-head/drupal-codesprint-2013
so tagging
Comment #70
YesCT CreditAttribution: YesCT commentedno change notice yet...
Something also like http://drupal.org/node/325974 Drupal SimpleTest coding standards please.
Comment #70.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #71
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedBest practices on Unit Agile Testing: http://drupal.org/node/1814344
Feel free to mention it. :-)
Comment #72
Wim LeersWithout 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:
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!)
Comment #73
tim.plunkettUnlike 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.
Comment #74
msonnabaum CreditAttribution: msonnabaum commentedWell, 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.
Comment #75
podarokremoving tag
Comment #76
BerdirThe 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 :)
Comment #77
ParisLiakos CreditAttribution: ParisLiakos commentedi think thats good enough.
did some changes
https://drupal.org/node/2012184/revisions/view/2716902/2717306
feel free to improve:)
Comment #78
ParisLiakos CreditAttribution: ParisLiakos commentedalso.
#1938068: Convert UnitTestBase to PHPUnit contains links to some conversions already happened, if someone wants to take a look
Comment #80
chx CreditAttribution: chx commentedFor 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.
Comment #80.0
chx CreditAttribution: chx commentednotes commits (two)
Comment #81
jhedstrom