[perl #42360] [TODO]: Unit tests for Parrot::Revision

0 views
Skip to first unread message

James Keenan

unread,
Apr 8, 2007, 9:07:44 PM4/8/07
to bugs-bi...@rt.perl.org
# New Ticket Created by James Keenan
# Please include the string: [perl #42360]
# in the subject line of all future correspondence about this issue.
# <URL: http://rt.perl.org/rt3/Ticket/Display.html?id=42360 >


lib/Parrot/Revision.pm appears to have no unit tests written
specifically for it. Write some, then perform coverage analysis to
make sure all code is covered by tests.

$ ack --nohtml --nophp 'Parrot::Revision'

config/gen/revision.pm:23:use Parrot::Revision;
config/gen/revision.pm:30: my $revision = $Parrot::Revision::current;
config/gen/revision.pm:31: my $entries =
$Parrot::Revision::svn_entries;
lib/Parrot/Revision.pm:6:Parrot::Revision - SVN Revision
lib/Parrot/Revision.pm:10: use Parrot::Revision;
lib/Parrot/Revision.pm:12: print $Parrot::Revision::current;
lib/Parrot/Revision.pm:13: print $Parrot::Revision::config;
lib/Parrot/Revision.pm:21:package Parrot::Revision;
t/distro/file_metadata.t:13:use Parrot::Revision;
t/distro/file_metadata.t:219: unless ( $Parrot::Revision::current
or `svk ls .` ) {
tools/build/revision_c.pl:25:use Parrot::Revision;
tools/build/revision_c.pl:46: return ${Parrot::Revision::current};
tools/build/revision_c.pl:51: return ${Parrot::Revision::config};

James Keenan via RT

unread,
May 1, 2007, 6:49:51 PM5/1/07
to perl6-i...@perl.org
In the course of writing tests for Parrot::Revision (see the 'reconfigure' branch), I noticed that
much of the code depends on the definedness and value of $svn_entries. However, since
$svn_entries is explicitly undefined at the top of lib/Parrot/Revision.pm, and since it's never
assigned to within that package, it's not clear what all that code is for.

Here's the result of ack-ing for svn_entries in trunk:

#####
[parrot] 502 $ ack --nohtml --nophp svn_entries


config/gen/revision.pm
31: my $entries = $Parrot::Revision::svn_entries;

lib/Parrot/Revision.pm
27:our $svn_entries = undef;
42: elsif ( defined $svn_entries and -r $svn_entries ) {
43: open FH, '<', $svn_entries
44: or die "Unable to open file ($svn_entries). Aborting. Error returned was: $!";
#####

If $svn_entries starts out undef-ed and is never assigned to, it can never pass the test for
definedness in line 42 above, which means that the subsequent elsif stanza can never be
reached -- and should therefore be refactored out of existence.

Is my interpretation correct?

Thank you very much.

kid51

Jerry Gay

unread,
May 1, 2007, 6:57:58 PM5/1/07
to parrotbug...@parrotcode.org, perl6-i...@perl.org
your interpretation is correct. '.svn_entries' is an svn metadata file
that's not even available in newer svn releases. Parrot::Revision
should never have been mucking with svn internals, anyway, and should
now be using the external api to get this info.

this code should be ripped out.
~jerry

James Keenan via RT

unread,
May 1, 2007, 10:40:49 PM5/1/07
to perl6-i...@perl.org
On Tue May 01 15:58:34 2007, particle wrote:
> > #####
> > [parrot] 502 $ ack --nohtml --nophp svn_entries
> > config/gen/revision.pm
> > 31: my $entries = $Parrot::Revision::svn_entries;
> >
> >
> your interpretation is correct. '.svn_entries' is an svn metadata file
> that's not even available in newer svn releases. Parrot::Revision
> should never have been mucking with svn internals, anyway, and should
> now be using the external api to get this info.
>
> this code should be ripped out.
> ~jerry
>

Okay, but we'll have to look at what to do with config/gen/revision.pm as well. It contains
this code:

sub runstep {
my ( $self, $conf ) = @_;

my $revision = $Parrot::Revision::current;

my $entries = $Parrot::Revision::svn_entries;

$conf->data->set(
revision => $revision,
SVN_ENTRIES => $entries
);
...

Here is the output of 'ack' for SVN_ENTRIES in trunk (post-Configure.pl, in this instance):

[parrot] 521 $ ack --nohtml --nophp SVN_ENTRIES
config/gen/revision.pm
35: SVN_ENTRIES => $entries

config_lib.pasm
274: set P0["SVN_ENTRIES"], P1

lib/Parrot/Config/Generated.pm
30: 'SVN_ENTRIES' => undef,

The 'undef' value for key 'SVN_ENTRIES' in the configuration hash is, now, not at all surprising
and will simply go away when we eliminate the offending code from lib/Parrot/Revision.pm.

But do we have to do anything with config_lib.pasm? (I've never patched any .pasm files).

Thanks.

kid51

James Keenan via RT

unread,
May 1, 2007, 11:23:03 PM5/1/07
to perl6-i...@perl.org
On Tue May 01 19:40:49 2007, jk...@verizon.net wrote:
>
>
> Here is the output of 'ack' for SVN_ENTRIES in trunk (post-
> Configure.pl, in this instance):
>
> [parrot] 521 $ ack --nohtml --nophp SVN_ENTRIES
...

> config_lib.pasm
> 274: set P0["SVN_ENTRIES"], P1
>
>
> But do we have to do anything with config_lib.pasm? (I've never
> patched any .pasm files).
>

I see that config_lib.pasm is generated by running Configure.pl. So its SVN_ENTRIES key goes
away as well.

kid51

James Keenan via RT

unread,
May 2, 2007, 10:33:18 PM5/2/07
to perl6-i...@perl.org
See attached patch revision.patch.txt. Per discussion on list with particle, lib/Parrot/
Revision.pm is revised to eliminate unassignable variable $svn_entries and one stanza of
code associated therewith. Also eliminates unused variable $ent. I also add 4 test files: two
in t/configure/ and two in t/postconfigure/. The latter tests are intended to be run after
Configure.pl has run because they presume the existence of lib/Parrot/Config/
Generated.pm; they SKIP otherwise.

I use Subversion as my local version control client, so I'm not currently able to write tests for
the svk-associated stanza. Hence, testing coverage is lower than I would normally be
satisfied with. Patches from svk users would be welcome.

As with my other patch submissions, unless I hear screams I'll apply to trunk in 3 days or so.
They've already been applied and tested in the 'reconfigure' branch.

revision.patch.txt

Chromatic

unread,
May 2, 2007, 11:36:44 PM5/2/07
to perl6-i...@perl.org, parrotbug...@parrotcode.org
On Wednesday 02 May 2007 19:33:18 James Keenan via RT wrote:
> I use Subversion as my local version control client, so I'm not currently
> able to write tests for the svk-associated stanza.  Hence, testing coverage
> is lower than I would normally be satisfied with.  Patches from svk users
> would be welcome.

Everything worked fine for me with Svk, barring some fuzz when updating
MANIFEST.SKIP and the lack of Svn properties set in the diff. Nice work.

-- c

James Keenan

unread,
May 3, 2007, 7:11:25 AM5/3/07
to parrotbug...@parrotcode.org

On May 2, 2007, at 11:37 PM, chromatic via RT wrote:

>
>
> Everything worked fine for me with Svk, barring some fuzz when
> updating
> MANIFEST.SKIP and the lack of Svn properties set in the diff. Nice
> work.
>

This was the first time that I used tools/dev/mk_manifest_and_skip.pl
to (re-)generate MANIFEST and SKIP. The diff itself was generated
via svn diff. What could I have done differently to get a cleaner
patch?

jimk

James Keenan via RT

unread,
May 5, 2007, 1:04:25 PM5/5/07
to perl6-i...@perl.org
Patch applied in r18427 May 05 2007.

James Keenan via RT

unread,
May 5, 2007, 1:05:59 PM5/5/07
to perl6-i...@perl.org
On Sat May 05 10:04:24 2007, jkeen <!-- x --> at verizon.net wrote:
> Patch applied in r18427 May 05 2007.

I'm leaving the ticket open in the hope that some project member who uses SVK for version
control can add tests for that VCS under t/configure/ and t/postconfigure/.

James Keenan via RT

unread,
May 13, 2007, 1:37:19 PM5/13/07
to perl6-i...@perl.org
On Wed May 02 19:33:17 2007, jkeen <!-- x --> at verizon.net wrote:
> See attached patch revision.patch.txt. Per discussion on list with
> particle, lib/Parrot/
> Revision.pm is revised to eliminate unassignable variable $svn_entries
> and one stanza of
> code associated therewith. Also eliminates unused variable $ent. I
> also add 4 test files: two
> in t/configure/ and two in t/postconfigure/. The latter tests are
> intended to be run after
> Configure.pl has run because they presume the existence of
> lib/Parrot/Config/
> Generated.pm; they SKIP otherwise.
>

I see that I should have also patched config/gen/revision.pm to eliminate this line:

my $entries = $Parrot::Revision::svn_entries;

...
SVN_ENTRIES => $entries

The patch attached accomplishes this.

kid51

config.gen.revision.patch.txt

James Keenan via RT

unread,
Sep 12, 2007, 8:50:05 PM9/12/07
to perl6-i...@perl.org

4 months later ... and no response from SVK users! Can anyone give me
some feedback here? (I'd like to close the ticket.)

James Keenan via RT

unread,
Jan 18, 2008, 10:47:44 PM1/18/08
to perl6-i...@perl.org

I'm throwing in the towel on the question of getting unit tests for the
SVK or GIT portions of this package. Closing ticket.

Reply all
Reply to author
Forward
0 new messages