Bug 247513 - Review Request: junit4 - java regression testing framework
Review Request: junit4 - java regression testing framework
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Fernando Nasser
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-07-09 13:32 EDT by Ben Konrath
Modified: 2007-11-30 17:12 EST (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-08-13 14:30:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fnasser: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
Detailed review (5.41 KB, text/plain)
2007-08-01 01:08 EDT, Vivek Lakshmanan
no flags Details
Suggested fixes for some of the issues (1.45 KB, patch)
2007-08-01 01:22 EDT, Vivek Lakshmanan
no flags Details | Diff

  None (edit)
Description Ben Konrath 2007-07-09 13:32:17 EDT
Spec URL: 

http://www.bagu.org/eclipse/junit4.spec

SRPM URL: 

http://www.bagu.org/eclipse/junit4-4.3.1-1jpp.1.fc7.src.rpm

Description:

JUnit is a regression testing framework written by Erich Gamma and Kent
Beck. It is used by the developer who implements unit tests in Java.
JUnit is Open Source Software, released under the IBM Public License and
hosted on SourceForge.

Additional Info:

The reason I am proposing a new junit4 package rather than updating the junit3 package is because eclipse 3.3 requires junit 3 and 4.
Comment 1 Vivek Lakshmanan 2007-07-30 15:57:03 EDT
I will start on the review...
Comment 2 Vivek Lakshmanan 2007-08-01 01:08:20 EDT
Created attachment 160393 [details]
Detailed review

Overview of things that need attention:
X * skim the summary and description for typos, etc.
  # The summary is outdated - references EPL which may confuse people. See
patch 
  # for fix from http://www.junit.org/index.htm
X * correct buildroot
 - should be:
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 # Needs to be fixed

X * license text included in package and marked with %doc
  # cpl-v10.html should be included in %doc

X * rpmlint on <this package>.srpm gives no output
  # tab and space mixing issues: see patch

X * run rpmlint on the binary RPMs
XNote: group warnings can be ignored.
# Results from binary rpm rpmlint-ing
#$ rpmlint -v /var/lib/mock/fedora-7-x86_64/root/builddir/build/RPMS/junit4-*
#I: junit4 checking
#W: junit4 wrong-file-end-of-line-encoding
/usr/share/doc/junit4-4.3.1/README.html
#I: junit4-debuginfo checking
#I: junit4-demo checking
#W: junit4-demo no-documentation
#I: junit4-javadoc checking
#I: junit4-manual checking
#W: junit4-manual wrong-file-end-of-line-encoding
/usr/share/doc/junit4-manual-4.3.1/faq/faq.htm
#W: junit4-manual wrong-file-end-of-line-encoding
/usr/share/doc/junit4-manual-4.3.1/testinfected/testing.htm
#W: junit4-manual wrong-file-end-of-line-encoding
/usr/share/doc/junit4-manual-4.3.1/index.htm
#W: junit4-manual wrong-file-end-of-line-encoding
/usr/share/doc/junit4-manual-4.3.1/cookbook/cookbook.htm
#W: junit4-manual wrong-file-end-of-line-encoding
/usr/share/doc/junit4-manual-4.3.1/cookstour/cookstour.htm

SHOULD:
X * package should include license text in the package and mark it with %doc
# See above, might need fixing
Comment 3 Vivek Lakshmanan 2007-08-01 01:22:02 EDT
Created attachment 160394 [details]
Suggested fixes for some of the issues

NEEDSWORK: See above summary
Comment 4 Ben Konrath 2007-08-01 17:14:50 EDT
New files:

http://bagu.org/eclipse/junit4.spec
http://bagu.org/eclipse/junit4-4.3.1-1jpp.2.fc8.src.rpm

(In reply to comment #2)
> Created an attachment (id=160393) [edit]
> Detailed review
> 
> Overview of things that need attention:
> X * skim the summary and description for typos, etc.
>   # The summary is outdated - references EPL which may confuse people. See
> patch 
>   # for fix from http://www.junit.org/index.htm

Fixed

> X * correct buildroot
>  - should be:
>    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>  # Needs to be fixed

Fixed

> X * license text included in package and marked with %doc
>   # cpl-v10.html should be included in %doc

This license file is not installed so it's not needed.

> X * rpmlint on <this package>.srpm gives no output
>   # tab and space mixing issues: see patch

Fixed

> X * run rpmlint on the binary RPMs

Fix most issues. Running rpmlint on the binary rpm now gives these messages:

%% rpmlint RPMS/x86_64/junit4-*
E: junit4 no-binary

This is ok because that package currently doesn't have binary files since
aot-compile-rpm is temporarily disabled due to gcj bugs.

E: junit4-debuginfo empty-debuginfo-package

Again, this package is empty because aot-compile-rpm is temporarily disabled due
to gcj bugs.

W: junit4-demo no-documentation

There is no documentation for the demo packages.
Comment 5 Vivek Lakshmanan 2007-08-06 23:07:32 EDT
(In reply to comment #4)
> New files:
> 
> http://bagu.org/eclipse/junit4.spec
> http://bagu.org/eclipse/junit4-4.3.1-1jpp.2.fc8.src.rpm
> 
Thanks and sorry for the delay. I dont think you need to bump the release for
this, but whatever :)

This is probably really nit-picky but I recommend using the 
%define _with_gcj_support 0 up top to disable aot compilation for the moment if
you want to. So you wont have to comment out ownership under the %{_libdir}/gcj
etc. and once the bugs in gcj are fixed simply switching the gcj_support thing
would do. Also, it seems that the version of spec-gcj-convert used to build the
spec has bugs:
%postun
%if %{gcj_support}
...
%endif

while it really should be:
%if %{gcj_support}
%postun
...
%endif

This leaves empty script sections when gcj support is disabled by default -
which is the case in jpackage and currently, due to the said gcj bug for Fedora
as well. Adding fnasser to CC since he is going to handle the push of the
package to jpackage.

NEEDSWORK: See above
Comment 6 Ben Konrath 2007-08-07 15:24:31 EDT
New Files:

http://bagu.org/eclipse/junit4-4.3.1-1jpp.1.fc8.src.rpm
http://bagu.org/eclipse/junit4.spec

(In reply to comment #5)
> Thanks and sorry for the delay. I dont think you need to bump the release for
> this, but whatever :)

Ok, I put it back to 1.

> This is probably really nit-picky but I recommend using the 
> %define _with_gcj_support 0 up top to disable aot compilation for the moment if
> you want to. So you wont have to comment out ownership under the %{_libdir}/gcj
> etc. and once the bugs in gcj are fixed simply switching the gcj_support thing
> would do. 

The macro wasn't working so I just set gcj_support to 0 manually.

> Also, it seems that the version of spec-gcj-convert used to build the
> spec has bugs:
> %postun
> %if %{gcj_support}
> ...
> %endif
> 
> while it really should be:
> %if %{gcj_support}
> %postun
> ...
> %endif
> 
> This leaves empty script sections when gcj support is disabled by default -
> which is the case in jpackage and currently, due to the said gcj bug for Fedora
> as well.

Fixed.
Comment 7 Fernando Nasser 2007-08-08 14:21:33 EDT
Starting import procedure
Comment 8 Fernando Nasser 2007-08-08 14:23:13 EDT
Confirmed that the changes requested by the reviewer were implemented. 
Requesting 'junit4' cvs branch for import.
Comment 9 Kevin Fenzi 2007-08-08 17:06:04 EDT
Can the submitter add a CVS template so we know what branches are needed, etc? 
See: http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure

Set the fedora-cvs flag back to ? when you are ready. 
Comment 10 Fernando Nasser 2007-08-09 11:00:25 EDT
Sorry, missed the template:

Package Change Request
======================
Package Name: junit4
New Branches: devel
Fedora Owners: fnasser@redhat.com, bkonrath@redhat.com
Comment 11 Kevin Fenzi 2007-08-09 17:31:13 EDT
I'm a little confused here... Vivek was reviewing this package, but Fernando
approved it and is going to maintain it as well? 

It's not typically allowed to approve a package that you are going to maintain. 
(With the idea a reviewer should be a disinterested party). 

Could we get a ack from Vivek that he approves this review and sees no further 
issues? 
Comment 12 Vivek Lakshmanan 2007-08-09 18:39:39 EDT
(In reply to comment #6)
> Fixed.

Thanks Ben, Everything looks good.
APPROVED.


Comment 13 Kevin Fenzi 2007-08-09 18:48:24 EDT
Sorry to be a pain about that...just want to make sure everything is aboveboard
and correct. :)

cvs done. 
Comment 14 Vivek Lakshmanan 2007-08-09 18:54:31 EDT
(In reply to comment #13)
> Sorry to be a pain about that...just want to make sure everything is aboveboard
> and correct. :)
> 
> cvs done. 
Thanks Kevin! Your concern was valid. I mentioned to Fernando elsewhere that
everything was OK with the package but he might have been a bit too eager to get
this out on rawhide :)
 


Comment 15 Fernando Nasser 2007-08-13 14:30:21 EDT
Built into Rawhide

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