Bug 785606 (Horde_Test) - Review Request: php-horde-Horde-Test - Horde testing base classes
Summary: Review Request: php-horde-Horde-Test - Horde testing base classes
Alias: Horde_Test
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
Depends On: horde-channel Horde_Util Horde_Support Horde_Log Horde_Cli
Blocks: Horde_Stream Horde_Secret Horde_Rpc Horde_Ldap 894563 Horde_Xml_Wbxml Horde_Xml_Element Horde_Text_Diff
TreeView+ depends on / blocked
Reported: 2012-01-29 22:16 UTC by Nick Bebout
Modified: 2013-03-21 15:43 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2012-12-12 22:49:32 UTC
Type: ---
fedora: fedora-review+
gwync: fedora-cvs+

Attachments (Terms of Use)
php-horde-Horde-Test-review.txt (13.23 KB, text/plain)
2012-08-02 13:30 UTC, Remi Collet
no flags Details

Description Nick Bebout 2012-01-29 22:16:36 UTC
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-1.fc16.src.rpm
Description: Horde-specific PHPUnit base classes.

Comment 1 Shawn Iwinski 2012-06-20 02:14:14 UTC
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
  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
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")

Comment 3 Shawn Iwinski 2012-06-23 12:27:03 UTC
(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)


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

Comment 5 Remi Collet 2012-08-02 13:30:51 UTC
Created attachment 601950 [details]

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

Comment 6 Remi Collet 2012-08-02 13:32:02 UTC
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.

Comment 7 Remi Collet 2012-08-02 13:32:37 UTC
No blocker

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

Comment 8 Nick Bebout 2012-08-02 20:24:41 UTC
New Package SCM Request
Package Name: php-horde-Horde-Test
Short Description: Horde-specific PHPUnit base classes
Owners: nb
Branches: el6 f16 f17

Comment 9 Gwyn Ciesla 2012-08-02 23:22:08 UTC
Git done (by process-git-requests).

Note You need to log in before you can comment on or make changes to this bug.