Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 0.6.0

Request #16393 Better support for file-level code
Submitted: 2009-07-01 07:36 UTC
From: digger Assigned: izi
Status: Closed Package: Testing_DocTest (version 0.4.1)
PHP Version: 5.2.9 OS: n/a
Roadmaps: (Not assigned)    
Subscription  


 [2009-07-01 07:36 UTC] digger (Dave Walton)
Description: ------------ Some of the files I would like to add doctests to contain bare code (outside of functions and classes), and have behavior that depends on external values (web server environment variables, command-line arguments, etc). Since DocTest require()s the tested source before running the test, the tested code has already executed by the time test code gets a chance to set up the necessary data. What would be nice to see is a keyword ("setup:" in my examples below) that could be used to specify test setup code to run BEFORE the tested file is require()d. This way the test code could prepare some necessary data, then the tested file would be require()d, and finally the test code would be run. Similarly, scripts with bare code like this will often produce output without any actual test code needed. But if the code section of the test is omitted, DocTest complains "Error: unexpected expects line". So the second part of this request is that the test code be optional. That is, a test could contain the setup code (described above), or the usual test code, or both, or neither. The "both" case is a bit tricky, due to the need to somehow separate the setup code and the test code. Perhaps there could be an optional keyword ("test:" in my examples) which if present just indicates the start of the test code, acting as a separator if necessary. Test script: --------------- /** * <code> * // doctest: Web page test * // setup: * // $_SERVER['REQUEST_URI'] = '/test/script'; * // expects: * // You requested '/test/script'. * </code> */ echo "You requested '{$_SERVER['REQUEST_URI']}'."; //=================== /** * <code> * // doctest: Command line test * // setup: * // $argc = 1; * // $argv = array('test'); * // expects: * // Your command was 'test'. * </code> */ if ($argc > 0) { echo "Your command was: '{$argv[0]}'."; } //=================== /** * <code> * // doctest: Both setup and test code * // setup: * // $_ENV['OSTYPE'] = 'linux'; * // test: * // echo OS_TYPE; * // expects: * // linux * </code> */ define('OS_TYPE', $_ENV['OSTYPE']);

Comments

 [2009-08-26 19:19 UTC] izi (David Jean Louis)
-Status: Open +Status: Feedback
Hi dave, Sorry for the delay. This is gonna be a bit complicated to implement, I'm currently working on other things and will not have time to implement it before long, would you like to try to work on a patch ? I'll be happy to review it ! Thanks, -- David
 [2009-09-16 10:37 UTC] digger (Dave Walton)
 [2009-09-16 10:42 UTC] digger (Dave Walton)
This patch is my attempt to implement this feature request. It turned out much simpler than I'd expected, which probably means I've missed any number of ghastly side-effects. It seems to do what I want it to do, but I'm not entirely certain what effect it'll have on existing tests. First of all, I noticed that a couple of my samples in the feature request have errors in them. In the second one, my expects: line doesn't match the 'echo' in the script. And I accidentally commented out the actual test code (echo OS_TYPE;) in the third one. Oops. The next thing I realized is that if setup: is implemented like clean:, with the code in comments, then there is no longer any need for the test: keyword I proposed. The setup: block would be in comments, followed by the uncommented test code. Following the existing clean: structure also gave me code to copy. With that change, my third sample looks like this: /** * <code> * // doctest: Both setup and test code * // setup: * // $_ENV['OSTYPE'] = 'linux'; * echo OS_TYPE; * // expects: * // linux * </code> */ define('OS_TYPE', $_ENV['OSTYPE']); Note that the only problem here is that if you try to comment out the first line of your test code, it'll effectively move up into the setup: block. But once I got into the code I realized that problem already exists for any keyword that precedes the test code. So if that counts as a bug, it's a pre-existing bug that I'll just ignore. :) Testing/DocTest/TestCase.php: Line 98: Initialized 'setupCode' to null. Testing/DocTest/Parser/Default.php Line 98: Add KW_DOCTEST_SETUP keyword. Line 145: Add STATE_SETUP state. Line 189: Add KW_DOCTEST_SETUP to keyword parse list. Line 237: Add a case for KW_DOCTEST_SETUP to call _handleSetupLine(). Line 580: Add STATE_SETUP to the list of states that expects: can follow. Note that this makes STATE_CODE optional, so that expects: can follow setup code, test code, or both. Note also that this does NOT allow the 'neither' case described in my feature request. I chose not to implement that, since it would mean allowing expects: to follow the flags:, doctest:, skip-if:, and (undocumented!) ini-set: keywords. Doing that would allow all kinds of undesireable ordering issues. And it's easy enough to put in a one-line dummy test code that it's not worth the hassle of dealing with those problems. Line 610: Add STATE_SETUP to the list of states that expects-file: can follow. The same notes above apply here, to. (Is expects-file: documented? I don't recall seeing it, but didn't really look.) Line 637: Add STATE_CLEAN to the list of states that test code cannot follow. I think not having it in this list is a bug. Line 720: Add _handleSetupLine() function, based on _handleCleanLine(). Collects setup: code into setupCode variable and sets state to STATE_SETUP. It disallows setup: after test code, expects:, expects-file:, and clean:. Line 773: Add a case for STATE_SETUP to call _handleSetupLine(). Testing/DocTest/Runner/Default.php Line 112: Add %s to $codetpl for injection of setupCode. Line 140: Add $testCase->setupCode to sprintf() call to inject it into $codetpl. At this point, it is possible that either $testCase->setupCode or $testCase->code will be null, unlike before where the expects: keyword required $testCase->code to be filled. However, I don't think it matters, since null should get put into %s as an empty string. And that's it. I really does look too easy. What did I miss?
 [2009-09-16 11:09 UTC] digger (Dave Walton)
 [2009-09-16 11:18 UTC] digger (Dave Walton)
After a bit more consideration, it occurs to me that perhaps I overestimated the ordering problems that could be caused by making the test code optional. If the allowed states are managed carefully, it might not be an issue. This second patch makes the test code optional by allowing expects: and expects-file: to appear after anything other than the other expects*: form or clean:. Since the only keyword that is allowed after either expects*: form is clean:, everything should be fine. But approach with caution.
 [2009-09-17 15:52 UTC] izi (David Jean Louis)
-Status: Feedback +Status: Assigned -Assigned To: +Assigned To: izi
Hi Dave, that's awesome ! I applied the patch and all tests pass, I had to modify the parser a bit because it would not work with multiple file level docblocks (it only executed the first one). I'm not sure about the second patch, is it to allow things like this ? /** * <code> * // doctest: Web page test * // setup: * // $_SERVER['REQUEST_URI'] = '/test/script'; * // expects: * // You requested '/test/script'. * </code> */ echo "You requested '{$_SERVER['REQUEST_URI']}'."; This is not going to work, because if the file echo something, all other tests in it will be broken. I'm not sure it's a good idea to support this anyway, let's forget it ;) First patch applied and commited: http://svn.php.net/viewvc?view=revision&revision=288401 a new release with your changes is coming soon ;) -- David
 [2009-09-18 00:16 UTC] digger (Dave Walton)
There are two things the second patch allows... First is the ultimate simple case of testing a script that does one thing, and so doesn't need any setup or test code. For example, a script that responds to an AJAX request with a list of countries from a database: /** * <code> * // flags: NORMALIZE_WHITESPACE, ELLIPSIS * // expects: * // <countries> * // [...] * // <country> * // <code>US</code> * // <name>United States</name> * // </country> * // [...] * // </countries> * </code> */ $data = getCountriesFromDatabase(); echo formatAsXML($data); In that case, you are correct, there can only be one test in the file. But not because other tests would be broken. The file only does one thing, and either succeeds or fails, so there isn't anything for more than one test to do. The second thing it allows is a more complex case of the simplified example in your comment. This is the more important and common of the two -- a web script that does different things based on a query arg: /** * <code> * // flags: NORMALIZE_WHITESPACE, ELLIPSIS * // setup: * // $_GET['action'] = 'getCountries'; * // expects: * // <countries>[...]</countries> * </code> * <code> * // flags: NORMALIZE_WHITESPACE, ELLIPSIS * // setup: * // $_GET['action'] = 'getLanguages'; * // expects: * // <languages>[...]</languages> * </code> */ switch ($_GET['action']) { case 'getCountries': $data = getCountriesFromDatabase(); echo formatAsXML($data); break; case 'getLanguages': $data = getLanguagesFromDatabase(); echo formatAsXML($data); break; } As you can see, in this case you CAN have multiple tests in the file, that test different parts of the code based on setup values. But there is no need for any test code, because the work has been done by the time the test code would have run. Examples like this are the main reason I needed the setup: keyword and wanted the test code optional. Currently in 0.4.1 you can write all kinds of tests for the functions called by this example, but this code itself is untestable. The critical fix is adding setup:, which makes this now testable. It's just a bit ugly to have to put in test code that does nothing, only to meet the requirement that test code be present. It'd be cleaner with the test code optional. But if you still think it's not worthwhile to do that, at least it can be worked around. :)
 [2010-04-10 15:12 UTC] izi (David Jean Louis)
-Status: Assigned +Status: Closed
Thank you for your bug report. This issue has been fixed in the latest released version of the package, which you can download at http://pear.php.net/get/