143155 - Make runtests.sh be a .pl

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, defect)

Description

[Zach Lipton [:zach]]

runtests.sh needs to be a perl script so it works under windows and so 
we can do other special stuff easier later on. This is also needed for 
newinstall.

Comment 1

[Jouni Heikniemi]

Are you going to tackle this soonish? I need to have the blocked bug 142744 done
fairly soon for my own uses, so if you don't have the time to do this soon, I
can take a look at it.

Comment 2

[Zach Lipton [:zach]]

I'll try to do this tonight, I leave for japan tomorrow and I will be emailess 
until the 31st so if I don't do it tonight, it's fair game

Comment 3

[Jouni Heikniemi]

Gotcha. :-) I'll do it if you won't have the time. Have a nice trip!

Comment 4

[Zach Lipton [:zach]]

feel free to take this on.

Comment 5

[mental]

Attachment: runtests.pl

Is there a way to turn on globbing with taint here?

Comment 6

[Jouni Heikniemi]

Unfortunately I can't run the test suite on Unix right now so I don't know if
this does the job there, but at least on Win32 this runs all the tests. It
outputs a horrendous amount of errors though, but I think that's the essence of
bug 142744. I don't think tainting is relevant here?

Two little comments: The patch should use four-space indents and checksetup.pl
should be patched for the filename change.

Are you going to do this? If so, please reassign the bug to yourself. If not,
say so and I'll do the fixes.

Comment 7

[Jouni Heikniemi]

[e-mail]:

Are you going to continue working on this patch? Please see my previous comment.

Comment 8

[Jouni Heikniemi]

Reassigning to patch author per discussion in mail.

Comment 9

[xyzzy]

Attachment: name changes

Comment 10

[xyzzy]

Attachment: indented

Okay, fixed known issues.

Comment 11

[Jouni Heikniemi]

The latest version is fine by me, too. Let's hope this gets some review
attention soon. 

Zach being away for the time being, I'll CC some of the other core folks for
reviews and possible comments.

Comment 12

[Gervase Markham [:gerv]]

Comment on attachment 83519 [details]
indented

>#!/usr/bonsaitools/bin/perl -w
># -*- Mode: perl; indent-tabs-mode: nil -*-
># ***** BEGIN LICENSE BLOCK *****

We tend not to use the license block form in Bugzilla. 
Just copy the license from another Bugzilla file.


># Make it harder for us to do dangerous things in Perl.
>use diagnostics;
>use strict;
>
>use Test::Harness qw(&runtests $verbose);
>
>$verbose=0;

Nit: $verbose = 0;

>foreach (@ARGV) {
>    if (/^--verbose$/) {
>        $verbose = 1;
>        last;
>    }
>}

Why "last"? It'll break things if another if() clause is added. Also,
can we have "-v" as a synonym for --verbose, please?

>runtests glob "t/*.t";

Can we use brackets here? And does the glob guarantee to return them in
numerical order?

Other than that, this is great. I had no idea it was so simple.

Gerv

Comment 13

[Jouni Heikniemi]

POSIX standard says the result of a glob is alphanumerically sorted. And Perl
glob  does it's job according to the standard - at least what they told me in
the docs some time ago ;-). So I guess that one's ok.

Comment 14

[xyzzy]

Attachment: updated

> We tend not to use the license block form in Bugzilla. 

Modified the one from checksetup.pl.  Hope it's right this time.  Can we add
something to the Developer's Guide on current practice regarding licenses?  I
hate having to guess when it comes to legal stuff...

> Nit: $verbose = 0;

Oh, duh!  Thought I'd fixed that.  Must have been just the =1 case. :P

> Why "last"? It'll break things if another if() clause is added. Also,
> can we have "-v" as a synonym for --verbose, please?

It allows early exit from the loop, so it's a good thing.  If somebody else
needs the change later, they can make it.  Synonym added.

> Can we use brackets here? And does the glob guarantee to return them in
> numerical order?

Parens added, and Jouni has addressed the ordering issue better than I could.

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 83766 [details]
updated

r=gerv.

Gerv

Comment 16

[xyzzy]

Attachment: smarter match

Sorry to spam you again.  This should fix the ignorant bug I introduced in the
last version.  Assuming r= still applies.

Comment 17

[Jouni Heikniemi]

Comment on attachment 83518 [details] [diff] [review]
name changes

r=jouni on the checksetup.pl changes.

Comment 18

[Jouni Heikniemi]

Comment on attachment 83799 [details]
smarter match

r=jouni

Still nothing to complain about. I've used this for its lifetime (both on Linux
and Win32) and it works fine. This should be ready for checkin as soon as
someone gives the minor checksetup.pl change patch a second r=.

Comment 19

[Gervase Markham [:gerv]]

Comment on attachment 83518 [details] [diff] [review]
name changes

r=gerv. This sort of thing, you can give 2xr= to :-)

Gerv

Comment 20

[Jouni Heikniemi]

I'll post to reviewers@ to find out the implications of this before checkin. 

Comment 21

[Zach Lipton [:zach]]

This is ready to go with review and all. I'm inclined to check this in, but
before we do so, I want to clarify one thing: do we "cvs remove" runtests.sh or
do we change runtests.sh to a little wrapper script that does something like:
"/usr/bonsaitools/bin/perl runtests.pl" ?

Comment 22

[Gervase Markham [:gerv]]

cvs remove it. Let's avoid backwards compatibility cruft where we can. runtests
is used only be developers; they can cope.

Gerv

Comment 23

[Zach Lipton [:zach]]

If we cvs remove runtests.sh, it will break tinderbox so we should probably 
do this when someone is around who can change the tinderbox 
commands to run .pl

Comment 24

[Gervase Markham [:gerv]]

We don't have to cvs remove runtests.sh at exactly the same moment we check in
runtests.pl. There can be a migration period. But we should remove runtests.sh
eventually.

Gerv

Comment 25

[Bradley Baetz (:bbaetz)]

What gerv said. Give it a week, at most.

Comment 26

[Zach Lipton [:zach]]

Patch is in, let's get tinderbox changed so I can cvs remove the .sh file
please. Dave?