Bug 785606 (Horde_Test)

I am not an official package reviewer, but here are some comments I have for this package:



*** It would help readability to group your Build* and Requires* statements together instead of mixing them with each other and your Provides and Conflicts statements



*** Your Provides statement should be
Provides:	php-pear(pear.horde.org/%{pear_name}) = %{version}
instead of
Provides:	php-pear(%{pear_name}) = %{version}



*** You should run phpci on your packages to make sure you have all requires:

$ phpci print --recursive --report=extension src/Horde_Test-1.3.0
22 / 22 [+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++>] 100.00%
BASE: /projects/fedora/REVIEWS/785606/src/Horde_Test-1.3.0
-------------------------------------------------------------------------------
PHP COMPAT INFO EXTENSION SUMMARY
-------------------------------------------------------------------------------
  EXTENSION                                        PECL   VERSION         COUNT
-------------------------------------------------------------------------------
  Core                                                    4.0.0              52
  PDO                                           1.0.4dev  5.1.0               1
  SPL                                                0.2  5.0.0               8
  dom                                           20031129  5.0.0               4
  json                                             1.2.1  5.2.0               1
  pcre                                                    4.0.0               4
  standard                                                4.0.0              63
-------------------------------------------------------------------------------
A TOTAL OF 7 EXTENSION(S) WERE FOUND
REQUIRED PHP 5.2.0 (MIN) 
-------------------------------------------------------------------------------
Time: 1 second, Memory: 9.25Mb
-------------------------------------------------------------------------------

According to phpci, you need to require "php-pdo" and "php-dom" (virtual provide of php-xml) at a minimum.  File Horde_Test-1.3.0/lib/Horde/Test/Functional.php uses the dom functions.  File Horde_Test-1.3.0/lib/Horde/Test/Factory/Db.php uses the PDO functions.  While package.xml lists these as optional, I believe this package would provide a better end-user experience if you added the two requires:

Requires: php-pdo
Requires: php-dom

spl, json, and pcre are all virtual provides from php-common. For completeness (and to prevent any future packaging issues due to PHP package changes) my sponsor and I have agreed to require all virtual provides.  It is up to you, but I would suggest adding the following as well:

Requires: php-spl
Requires: php-json
Requires: php-pcre



*** Do you plan on building for EPEL 5?
- If not, please remove "rm -rf $RPM_BUILD_ROOT" from the %install section (see http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag).
- If you do, there are several staements that need to be added.



*** Up to you, but package.xml lists the following optional packages if you choose to require:
- php-pear(pear.horde.org/Horde_Cli)
- php-pear(pear.horde.org/Horde_Log)
(if you do choose to require these packages, please update this ticket's "Depends On")

I believe I have fixed these issues.

Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test.spec
SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test-1.3.0-2.fc16.src.rpm

(In reply to comment #2)

Almost ready for official review. Please change

Requires:	php-pear(pear.horde.org/Horde_Cli)
Requires:	php-pear(pear.horde.org/Horde_Log)

to 

Requires:	php-pear(pear.horde.org/Horde_Cli) < 2.0.0
Requires:	php-pear(pear.horde.org/Horde_Log) < 2.0.0

per package.xml requirements

I believe I have fixed these issues.

Spec URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test.spec
SRPM URL: http://nb.fedorapeople.org/horde-reviews/php-horde-Horde-Test-1.3.0-3.fc16.src.rpm

Created attachment 601950 [details]
php-horde-Horde-Test-review.txt

Generated by fedora-review 0.2.0 (53cc903) last change: 2012-07-09

Could: remove "localized" php.ini (no more useful)

Could: requires php(language) >= 5.2.0 per new PHP Guildelines, 
but this is fedora specific (for now), so php-common seems acceptable as you target both fedora/epel

MUST Package installs properly:
Need to wait for Horde_Log and Horde_Constrant to be approved before import

Lot of used class are not listed in requires 
As they are not listed by upstream, I think we can ommit them (optional)

Ex, in /usr/share/pear/Horde/Test/Factory/Alarm.php
        if (!class_exists('Horde_Alarm')) {
            throw new Horde_Test_Exception('The "Horde_Alarm" class is unavailable!');
        }
        return new Horde_Alarm_Null();

Package with tests which use the Horde_Test_Factory_Alarm will have to BR both Horde_Test and Horde_Alarm.

After this package, I think others horde package, providing tests, will have to run them in the %check.

No blocker

========= APPROVED =========

New Package SCM Request
=======================
Package Name: php-horde-Horde-Test
Short Description: Horde-specific PHPUnit base classes
Owners: nb
Branches: el6 f16 f17
InitialCC:

Git done (by process-git-requests).