Req #16393 [Com]: Better support for file-level code

Edit report at http://pear.php.net/bugs/bug.php?id=16393&edit=1 ID: 16393 Comment by: dw-pear@digger.net Reported By: dw-pear at digger dot net Summary: Better support for file-level code Status: Assigned Type: Feature/Change Request Package: Testing_DocTest Operating System: n/a Package Version: 0.4.1 PHP Version: 5.2.9 Assigned To: izi Roadmap Versions: New Comment: 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. :) Previous Comments: ------------------------------------------------------------------------ [2009-09-17 11:52:54] izi -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-16 07:18:52] digger 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-16 07:09:18] digger Added #patch bug:16393;patch:test-code-optional;revision:1253081358;. ------------------------------------------------------------------------ [2009-09-16 06:42:59] digger 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 06:37:30] digger Added #patch bug:16393;patch:add-setup-keyword;revision:1253079450;. ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at http://pear.php.net/bugs/bug.php?id=16393 -- Edit this bug report at http://pear.php.net/bugs/bug.php?id=16393&edit=1