ZF-2334: Promote Zend_Service_Technorati to core for release
Description
Once unit testing for each file of the component reaches 80% LOC coverage and there exists satisfactory DocBook documentation for the APIs, the Zend_Service_Technorati component needs to be promoted to core (i.e., moved to trunk with unit tests and documentation integrated).
Comments
Posted by Simone Carletti (weppos) on 2007-12-23T19:32:34.000+0000
r7247 includes 100% of Technorati API support and 90% of unit tests.
Next step is to reach 99% of unit tests. It shouldn't be hard, all classes have been stressed by custom tests thus I only need to validate them, no more changes should be required.
Before starting the documentation it would be great to receive an overall feedback about the final product. :)
Posted by Simone Carletti (weppos) on 2007-12-25T17:31:42.000+0000
@ Darby
With r7263, I feel the library is ready. I've included 99% of unit tests and full documentation.
What do you think? :)
Posted by Darby Felton (darby) on 2007-12-26T07:41:13.000+0000
Great! I'll review this today and post again here. Thanks for these contributions! :)
Posted by Darby Felton (darby) on 2007-12-27T16:05:55.000+0000
Sorry, Simone, for the delay in getting back with you... I haven't forgotten! :) I plan to take a look first thing tomorrow and post again here.
Posted by Darby Felton (darby) on 2007-12-28T08:23:31.000+0000
I get a fatal error when running the unit tests on r7284 with PHP 5.1.4 (WinXP):
$ tail -f Zend_Service_Technorati_AllTests.out PHPUnit 3.2.8 by Sebastian Bergmann. Zend Framework - Zend_Service_Technorati Zend_Service_Technorati_TechnoratiTest ...................... Fatal error: Wrong parameters for Exception([string $exception [, long $code ]]) in trunk\incubator\library\Zend\Service\Technorati\Utils.php on line 67 Call Stack: 0.0054 1. {main}() c:\wamp\php\PEAR\PHPUnit\TextUI\Command.php:0 1.8072 2. PHPUnit_TextUI_Command::main() c:\wamp\php\PEAR\PHPUnit\TextUI\Command.php:528 3.0258 3. PHPUnit_TextUI_TestRunner->doRun() c:\wamp\php\PEAR\PHPUnit\TextUI\Command.php:112 3.0294 4. PHPUnit_Framework_TestSuite->run() C:\wamp\php\PEAR\PHPUnit\TextUI\TestRunner.php:298 3.0322 5. PHPUnit_Framework_TestSuite->run() C:\wamp\php\PEAR\PHPUnit\Framework\TestSuite.php:633 17.7259 6. PHPUnit_Framework_TestSuite->runTest() C:\wamp\php\PEAR\PHPUnit\Framework\TestSuite.php:650 17.7259 7. PHPUnit_Framework_TestCase->run() C:\wamp\php\PEAR\PHPUnit\Framework\TestSuite.php:670 17.7259 8. PHPUnit_Framework_TestResult->run() C:\wamp\php\PEAR\PHPUnit\Framework\TestCase.php:351 17.7288 9. PHPUnit_Framework_TestCase->runBare() C:\wamp\php\PEAR\PHPUnit\Framework\TestResult.php:597 17.8634 10. PHPUnit_Framework_TestCase->runTest() C:\wamp\php\PEAR\PHPUnit\Framework\TestCase.php:376 17.8657 11. ReflectionMethod->invoke() C:\wamp\php\PEAR\PHPUnit\Framework\TestCase.php:449 17.8658 12. Zend_Service_Technorati_TechnoratiTest->testBlogInfo() trunk\incubator\tests\Zend\Service\Technorati\TechnoratiTest.php:0 17.8854 13. Zend_Service_Technorati->blogInfo() trunk\incubator\tests\Zend\Service\Technorati\TechnoratiTest.php:327 18.0550 14. Zend_Service_Technorati_BlogInfoResult->__construct() trunk\incubator\library\Zend\Service\Technorati.php:371 18.1702 15. Zend_Service_Technorati_Utils::setUriHttp() trunk\incubator\library\Zend\Service\Technorati\BlogInfoResult.php:106 18.1927 16. Exception->__construct() trunk\incubator\library\Zend\Service\Technorati\Utils.php:67Posted by Simone Carletti (weppos) on 2007-12-28T08:47:53.000+0000
On my Mac it works. :|
I'm going to investigate the error. First I'll upgrade my phpunit installation, I see you have 3.2.8.
I'll be back in a moment...
Posted by Simone Carletti (weppos) on 2007-12-28T08:54:12.000+0000
I've just committed an update, it should be fixed now. Could you please let me know your PHP version?
Posted by Darby Felton (darby) on 2007-12-28T09:12:50.000+0000
The PHP version I noted above as 5.1.4.
With r7285 I now get the following fatal error:
PHPUnit 3.2.8 by Sebastian Bergmann. Zend Framework - Zend_Service_Technorati Zend_Service_Technorati_TechnoratiTest .......................................... Zend_Service_Technorati_AuthorTest . Fatal error: Argument 1 passed to Zend_Service_Technorati_Author::__construct() must be an object of class DomElement, called in trunk\incubator\tests\Zend\Service\Technorati\AuthorTest.php on line 63 and defined in trunk\incubator\library\Zend\Service\Technorati\Author.php on line 95 Call Stack: 0.0042 1. {main}() c:\wamp\php\PEAR\PHPUnit\TextUI\Command.php:0 1.9327 2. PHPUnit_TextUI_Command::main() c:\wamp\php\PEAR\PHPUnit\TextUI\Command.php:528 3.2590 3. PHPUnit_TextUI_TestRunner->doRun() c:\wamp\php\PEAR\PHPUnit\TextUI\Command.php:112 3.2601 4. PHPUnit_Framework_TestSuite->run() C:\wamp\php\PEAR\PHPUnit\TextUI\TestRunner.php:298 25.3584 5. PHPUnit_Framework_TestSuite->run() C:\wamp\php\PEAR\PHPUnit\Framework\TestSuite.php:633 25.4709 6. PHPUnit_Framework_TestSuite->runTest() C:\wamp\php\PEAR\PHPUnit\Framework\TestSuite.php:650 25.4710 7. PHPUnit_Framework_TestCase->run() C:\wamp\php\PEAR\PHPUnit\Framework\TestSuite.php:670 25.4710 8. PHPUnit_Framework_TestResult->run() C:\wamp\php\PEAR\PHPUnit\Framework\TestCase.php:351 25.4731 9. PHPUnit_Framework_TestCase->runBare() C:\wamp\php\PEAR\PHPUnit\Framework\TestResult.php:597 25.5085 10. PHPUnit_Framework_TestCase->runTest() C:\wamp\php\PEAR\PHPUnit\Framework\TestCase.php:376 25.5105 11. ReflectionMethod->invoke() C:\wamp\php\PEAR\PHPUnit\Framework\TestCase.php:449 25.5105 12. Zend_Service_Technorati_AuthorTest->testConstructThrowsExceptionWithInvalidDom() trunk\incubator\tests\Zend\Service\Technorati\AuthorTest.php:0 25.5121 13. Zend_Service_Technorati_Author->__construct() trunk\incubator\tests\Zend\Service\Technorati\AuthorTest.php:63Posted by Simone Carletti (weppos) on 2007-12-28T09:25:19.000+0000
Mmm, this should be a problem.
Yes, I saw the version when the message was already posted and I haven't permission to edit my comments.
About the error, this is a strange behavior:
As you can see, I test whether the method returns an exception if the constructor is called with an invalid argument. The error should be catched by try/catch block, and in fact it is in my environment.
Do you have an idea why PHP 5.1.4 isn't aware of try/catch block here?
Posted by Darby Felton (darby) on 2007-12-28T09:33:33.000+0000
I haven't pored over the code yet, but it seems that this fatal error is not the result of an uncaught exception. Instead, a failure to meet a type hint in PHP < 5.2.0, I believe, results in a non-catchable fatal error.
Posted by Simone Carletti (weppos) on 2007-12-28T09:40:39.000+0000
The only alternative I can see right now is to skip all these tests whether PHP < 5.2.0. This one is not the only test passing an invalid constructor but almost each class is tested against an invalid call.
What do you think?
Posted by Darby Felton (darby) on 2007-12-28T09:44:52.000+0000
Yes, this is what was done with other unit tests of the same type (e.g., see {{Zend_Db_Adapter_StaticTest}} and look for {{PHP_VERSION}}).
Posted by Darby Felton (darby) on 2007-12-28T09:59:28.000+0000
Now that I look into it more, it seems that I have based my assertion that type hinting failure results in non-catchable fatal errors on another person's incomplete work. I recommend that if you want to test failure to satisfy a type hint, try using a custom error handler.
Posted by Darby Felton (darby) on 2007-12-28T10:16:43.000+0000
Okay, maybe I should not have second-guessed myself. :) Or am I now doubting my doubts? ;) I ran the following script on my machine:
Results:
Fatal error: Argument 1 passed to foo() must be an object of class stdClass, called in C:\wamp\www\test.php on line 19 and defined in C:\wamp\www\test.php on line 13
I'm pretty sure that this means that at least on my machine running 5.1.4, the failure to meet the type hint does not result in a catchable fatal error.
Posted by Simone Carletti (weppos) on 2007-12-28T10:19:19.000+0000
I've just updated unit tests. Now "invalid argument" tests are skipped if PHP < 5.2.0.
I'm going to install and configure PHP 5.1.4 in the next few days to be able to develop with this specific release.
Posted by Darby Felton (darby) on 2007-12-28T12:13:59.000+0000
I get very good LOC coverage percentages from the unit tests, though I see the following anomalies under {{library/Zend/Service/Technorati}}:
|ResultSet.php|52.85%| |TagsResultSet.php|66.67%| |Utils.php|61.11%|
Posted by Simone Carletti (weppos) on 2007-12-28T13:08:49.000+0000
I've just installed xdebug and run the test coverage report.
Utils.php includes 3 methods defined but never implemented, this is the reason. They are utility methods useful for DRY development, perhaps I have to comment them for now.
ResultSet.php is not a class intended to be used alone, as Result.php. It should be tested directly from parent classes, hovewer I'm going to have a look at it.
TagsResultSet.php has 2 methods, one is the constructor. The second method is missing a test method, this is why the class has about 50% of code coverage.
I'm going to fix missing tests in the next hours. Any other overall feedback? :)
Posted by Darby Felton (darby) on 2007-12-28T13:55:02.000+0000
Should {{Zend_Service_Technorati_ResultSet}} be designated as {{abstract}}, since the documentation states:
bq. ...it should never be used to initialize a standalone object
Posted by Darby Felton (darby) on 2007-12-28T13:58:15.000+0000
...same with {{Zend_Service_Technorati_Result}}.
Posted by Darby Felton (darby) on 2007-12-28T14:52:03.000+0000
{quote} Utils.php includes 3 methods defined but never implemented, this is the reason. They are utility methods useful for DRY development, perhaps I have to comment them for now. {quote}
There are a couple other solutions to consider for these methods:
I think that I would prefer the first approach over the second.
Posted by Darby Felton (darby) on 2007-12-28T14:54:10.000+0000
Oops, a correction to my previous comment:
-phpdoc tags- +PHPUnit annotations+
Posted by Simone Carletti (weppos) on 2007-12-28T16:03:07.000+0000
I collected all your feedbacks and committed an update. I updated the manual to reflect some public changes (I forgot to remove a public property from ResultSet class).
Posted by Darby Felton (darby) on 2007-12-31T14:08:14.000+0000
I just committed some small changes in SVN r7318 to get the code coverage of the Utils class above 80%. I notice there are quite a few things marked "{{@todo}}" in the sources...
Under {{library/}}:
./Technorati/CosmosResult.php: * @todo Each field needs to be filtered and converted ./Technorati/CosmosResult.php: * @todo filter as Zend_Uri_Http ./Technorati/CosmosResult.php: * @todo Zend_Date ./Technorati/CosmosResult.php: * @todo Zend_Uri_Http ./Technorati/CosmosResultSet.php: // @todo Improve xpath expressions ./Technorati/DailyCountsResultSet.php: // @todo Improve xpath expressions ./Technorati/KeyInfoResult.php: /** @todo improve xpath expression */ ./Technorati/Result.php: * @todo XPath and DOM elements cannot be serialized, don't cache them ./Technorati/ResultSet.php: * @todo ./Technorati/ResultSet.php: * @todo XPath and DOM elements cannot be serialized, don't cache them ./Technorati/ResultSet.php: * @todo XPath and DOM elements cannot be serialized, don't cache them ./Technorati/ResultSet.php: // @todo Use constants ./Technorati/ResultSet.php: // @todo Improve xpath expression ./Technorati/SearchResult.php: * @todo Zend_Date ./Technorati/SearchResult.php: * @todo Zend_Uri_Http ./Technorati/SearchResult.php: * @todo Each field needs to be filtered and converted ./Technorati/SearchResultSet.php: // @todo Improve xpath expressions ./Technorati/TagResult.php: * @todo Zend_Date ./Technorati/TagResult.php: * @todo Zend_Date ./Technorati/TagResult.php: * @todo Zend_Uri_Http ./Technorati/TagResult.php: * @todo Each field needs to be filtered and converted ./Technorati/TagResultSet.php: // @todo Improve xpath expressions ./Technorati/TagResultSet.php: /** @todo Validate the following assertion */ ./Technorati/Utils.php: * @todo Should this method return a value or set the value? ./Technorati/Utils.php: * @todo public static function xpathQueryAndSet() {} ./Technorati/Utils.php: * @todo public static function xpathQueryAndSetIf() {} ./Technorati/Utils.php: * @todo public static function xpathQueryAndSetUnless() {} ./Technorati/Weblog.php: * @todo Zend_Date ./Technorati.php: * @todo support for Zend_Uri_HttpUnder {{tests/}}:
I think these should be reduced to only those "{{@todos}}" that cannot reasonably be addressed prior to promotion and release.
If some of the issues are large enough to be worth documenting, perhaps for these you can create sub-tasks or new issues on which this one would depend?
Posted by Simone Carletti (weppos) on 2008-01-01T08:39:38.000+0000
Hi Darby, happy new Year! ;)
I saw your changes and I agree with all of them except one I'd like to ask you for more details. Here you removed the second piece of code with the explanation
{quote} * removed code impossible to execute from setUriHttp() method {quote}
This is true right now, but according to Shahar's Zend_Uri improvements proposal Zend_Uri is going to support additional schemas. Even if this is just a proposal, is reasonable to suppose that the future of Zend_Uri is open to new schemas thus the second check was created to be sure the result of
```
is a valid instance of a Zend_Uri_Http and not, for example, Zend_Uri_Mailto. Unit tests are good enough to point out the error as soon as Zend_Uri will support more protocols, but restoring the control will prevent a 'last minute fix'. What do you think?
About @todos you are right, there's an high number of @todo but just because I usually first develop the basic feature and I like to add notes to remember the feature might be improved in a second time. I mean, @todos don't mean the code doesn't work, only it could have a better interaction with the environment (for example, @todo Zend_Uri_Http). ;)
I'm going to review the code addressing first all todos that may fit the classification 'should have', then I will create a new issue for any 'nice to have' as I did, for example, for ZF-2350.
Posted by Simone Carletti (weppos) on 2008-01-01T11:41:16.000+0000
Darby, I've cleared/fixed all reasonable @todos, added more unit tests focused on SeekableIterator interface.
Just a final question: should Zend_Service_Technorati_ResultSet#getXML() be renamed to Zend_Service_Technorati_ResultSet#getXml(), isn't it?
Posted by Darby Felton (darby) on 2008-01-02T15:34:46.000+0000
bq. I saw your changes and I agree with all of them except one I'd like to ask you for more details. Here you removed the second piece of code...
Oops, I had in mind that the check was against {{Zend_Uri}} and not {{Zend_Uri_Http}}. Sorry I misunderstood. Please feel free to resurrect that section of code, or if you'd like, I can do it. :)
bq. I've cleared/fixed all reasonable @todos, added more unit tests focused on SeekableIterator interface.
Awesome; thanks! :)
bq. Just a final question: should Zend_Service_Technorati_ResultSet#getXML() be renamed to Zend_Service_Technorati_ResultSet#getXml(), isn't it?
Yes, it should be {{getXml()}} according to the naming conventions in the coding standards.
I'll perform what I hope might be a final review as soon as possible. I also suggest contacting the fw-webservices mailing list (and forwarding to fw-general, perhaps) to give the code a whirl to test it and provide feedback, to catch any lingering API issues, etc. that should be addressed prior to release with the framework core.
Posted by Simone Carletti (weppos) on 2008-01-02T17:22:56.000+0000
Fixes committed in r7333.
Posted by Simone Carletti (weppos) on 2008-01-15T02:37:49.000+0000
Hi Darby, I was wondering if you are going to move the component to core before the upcoming code freeze. :)
Posted by Darby Felton (darby) on 2008-01-29T15:46:52.000+0000
Attached test output from HEAD of the {{trunk/}} (PHP 5.1.4, WinXP)
Posted by Simone Carletti (weppos) on 2008-01-30T02:29:29.000+0000
Gasp, I'm absolutely shocked by the output. It seems something goes wrong with Zend_Date, I will investigate.
Posted by Simone Carletti (weppos) on 2008-01-30T02:52:36.000+0000
Hi Darby, I investigated the issue and, as I supposed, it's related to Zend_Date. It seems Zend_Date no longer recognizes valid dates (ZF-2524).
I'm waiting for a Thomas's answer to check what is the origin of the issue. The last time I run the suite (02/Jan/08), all tests passed without any problem.
Posted by Darby Felton (darby) on 2008-01-30T13:28:27.000+0000
Yes, it seems as though something has changed with Zend_Date, as I have never seen such errors in the Zend_Service_Technorati tests until now.
Probably it would be a simple matter to provide the date format, as Thomas suggests, to get the tests passing again.
I am concerned, however, that there has been a backward compatibility break with Zend_Date, inasmuch as people have been depending on certain behavior that is now broken. :( That said, perhaps it is not a backward compatibility break in the sense that perhaps the usage within Zend_Service_Technorati was broken, non-standard, not documented, or otherwise unsupported behavior. Thomas could probably shed some light on this. All things considered, I don't really understand why {{Zend_Date::isDate()}} cannot grok the same date value that {{new Zend_Date()}} supports. Strange, indeed.
Posted by Simone Carletti (weppos) on 2008-01-30T16:15:24.000+0000
I found the guilty, the change that broke BC compatibility (at least in Zend_Service_Technorati case). I'm waiting for Thomas answer.
It would be hard to provide date format because Technorati is not so coherent! You should consider that they created an own DTD but the most part of API XML responses is invalid against their own DTD!
Posted by Simone Carletti (weppos) on 2008-02-01T04:35:49.000+0000
I fixed the issue with r7751 using strtotime to validate the input.
Posted by Darby Felton (darby) on 2008-02-01T10:21:34.000+0000
Hi Simone,
It looks as though Zend_Service_Technorati is ready to be moved to core. Are you aware of anything that should be done beforehand?
Would you like to migrate the component, or would you like me to do it on your behalf?
Thanks for these excellent contributions! :)
Posted by Simone Carletti (weppos) on 2008-02-01T10:30:24.000+0000
Hi Darby, according to official documentation only a core contributor could promote a component to core this is why I never moved a single bit. ;)
By the way, if you want I can move it and left you free for other activities. I suppose I only need to * svn move incubator/library incubator/test to /library /test * promote documentation * update docbook single files and main index. I guess there's nothing more to do, isn't it?
Posted by Darby Felton (darby) on 2008-02-01T11:32:43.000+0000
Sorry, Simone, I didn't realize that it was documented that only a Zender could perform the actual promotion. I have confidence that you would have no problem doing it, but in the interest of following the rules, I'll go ahead and make promote it today as soon as possible. :)
Thanks again!
Posted by Darby Felton (darby) on 2008-02-01T12:08:00.000+0000
Resolved with SVN r7757.