Issues

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

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. :)

@ Darby

With r7263, I feel the library is ready. I've included 99% of unit tests and full documentation.

What do you think? :)

Great! I'll review this today and post again here. Thanks for these contributions! :)

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.

I get a fatal error when running the unit tests on r7284 with PHP 5.1.4 (WinXP):

$ phpunit --verbose --report Zend_Service_Technorati_AllTests Zend_Service_Technorati_AllTests > Zend_Service_Technorati_AllTests.out 2>&1 &
$ 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:67

On my Mac it works. :|


weppos-mac:~/Sites/svn/zend.com.framework/trunk/incubator/tests weppos$ phpunit --verbose Zend_Service_Technorati_AllTests
PHPUnit 3.2.5 by Sebastian Bergmann.

Zend Framework - Zend_Service_Technorati
 Zend_Service_Technorati_TechnoratiTest
 ..........................................

 Zend_Service_Technorati_AuthorTest
 ....

 Zend_Service_Technorati_WeblogTest
 .....

 Zend_Service_Technorati_BlogInfoResultTest
 ....

 Zend_Service_Technorati_GetInfoResultTest
 ...

 Zend_Service_Technorati_KeyInfoResultTest
 .....

 Zend_Service_Technorati_CosmosResultTest
 .......

 Zend_Service_Technorati_CosmosResultSetTest
 .........

 Zend_Service_Technorati_DailyCountsResultTest
 ...

 Zend_Service_Technorati_DailyCountsResultSetTest
 ...

 Zend_Service_Technorati_SearchResultTest
 ....

 Zend_Service_Technorati_SearchResultSetTest
 ...

 Zend_Service_Technorati_TagResultTest
 ...

 Zend_Service_Technorati_TagResultSetTest
 ...

 Zend_Service_Technorati_TagsResultTest
 ....

 Zend_Service_Technorati_TagsResultSetTest
 ...


Time: 1 second


OK (105 tests)

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...

I've just committed an update, it should be fixed now. Could you please let me know your PHP version?

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:63

Mmm, this should be a problem.

The PHP version I noted above as 5.1.4.

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:


    public function testConstructThrowsExceptionWithInvalidDom() 
    {
        try {
            $object = new Zend_Service_Technorati_Author('foo');
            $this->fail('Expected Zend_Service_Technorati_Exception not thrown');
        } catch (Exception $e) {
            $this->assertContains("DOMElement", $e->getMessage());
        }
    }

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?

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.

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?

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}}).

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.

Okay, maybe I should not have second-guessed myself. :) Or am I now doubting my doubts? ;) I ran the following script on my machine:


<?php

error_reporting(E_ALL | E_STRICT);

set_error_handler('errorHandler');

function errorHandler($errno, $errstr, $errfile, $errline, $errcontext)
{
    echo 'errorHandler() called';
    throw new Exception($errstr, $errno);
}

function foo(stdClass $obj)
{
    echo 'foo';
}

try {
    foo('scalar');
} catch (Exception $e) {
    echo 'Caught exception';
}

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.

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.

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%|

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? :)

Should {{Zend_Service_Technorati_ResultSet}} be designated as {{abstract}}, since the documentation states:

bq. ...it should never be used to initialize a standalone object

...same with {{Zend_Service_Technorati_Result}}.

{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:

  • Keep them available, and use unit tests to demonstrate what they /would/ do, if they were implemented as intended.
  • Keep them available, but using {{@codeCoverageIgnoreStart}} and {{@codeCoverageIgnoreEnd}} phpdoc tags.

I think that I would prefer the first approach over the second.

Oops, a correction to my previous comment:

-phpdoc tags- +PHPUnit annotations+

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).

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_Http

Under {{tests/}}:

./Technorati/SearchResultTest.php:        // @todo Zend_Uri_Http
./Technorati/SearchResultTest.php:        // @todo Zend_Date
./Technorati/TagResultTest.php:        // @todo Zend_Uri_Http
./Technorati/TagResultTest.php:        // @todo Zend_Date
./Technorati/TechnoratiTest.php:                 * @todo    validate converted value
./Technorati/WeblogTest.php:         * @todo lat, lon, hasphoto

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?

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.

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?

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.

Fixes committed in r7333.

Hi Darby, I was wondering if you are going to move the component to core before the upcoming code freeze. :)

Attached test output from HEAD of the {{trunk/}} (PHP 5.1.4, WinXP)

Gasp, I'm absolutely shocked by the output. It seems something goes wrong with Zend_Date, I will investigate.

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.

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.

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!

I fixed the issue with r7751 using strtotime to validate the input.

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! :)

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?

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!

Resolved with SVN r7757.