module-build-general

[Module-build-general] RFC: Build test test_dirs=(list)

[David C. <dl...@ha...>]

When working on a large distribution with many packages/modules and
tests, I often don't want to wait for all tests in the suite to run when
I'm working deep in the heirarchy.  Especially when I'm pretending to
practice Extreme Programming.

Just today I've created a builder subclass and have overridden the
test_files() method to look for $self->{args}{test_dirs} and add all the
*.t files found in or beneath those directories;  IMO much more convenient
than typing a long list for $self->{args}{test_files}.  So for instance
when I'm creating the Foo::Bar::Baz::Feh module in package Foo::Bar,
all the tests for Feh live in t/Foo/Bar/Baz/Feh, and I run "Build test
test_dirs=t/Foo/Bar/Baz/Feh".

I'm trying to think of a reason not to have it add *.pl as well....?

I'd like to see something like this incorporated into M::B.  It's a
trivial patch that I'd be happy to submit.

Comments....?

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Michael G S. <sc...@po...>]

On Mon, Aug 11, 2003 at 03:18:10PM -0700, David Carmean wrote:
> Just today I've created a builder subclass and have overridden the
> test_files() method to look for $self->{args}{test_dirs} and add all the
> *.t files found in or beneath those directories;  IMO much more convenient
> than typing a long list for $self->{args}{test_files}.  So for instance
> when I'm creating the Foo::Bar::Baz::Feh module in package Foo::Bar,
> all the tests for Feh live in t/Foo/Bar/Baz/Feh, and I run "Build test
> test_dirs=t/Foo/Bar/Baz/Feh".

The trouble with doing this by default is it makes this sort of testing
very difficult:

    t/foo.t
    t/Dummy-Module/lib/Dummy/Module.pm
    t/Dummy-Module/t/Foo.t

where foo.t needs a little module to test with.  If you go recursive in
scanning for .t files you'll pick up Foo.t.  There are workarounds but
they're similar to yours above so you're just shifting the problem.

What might be better is a simple flag you can pass to Module::Build->new()
which says "check recursively for tests".  Whether or not this should be
the default is a matter to be settled by a steel cage match.


> I'm trying to think of a reason not to have it add *.pl as well....?

The more things you look for as tests the harder it will be to have testing 
data in t/.  Its desirable to keep the number of magical files to a minimum 
else you magic it difficult to shake off that magic.

Since there's no compelling use-case for making *.pl magical other than "Look, 
we can name it something else!" I'd say no.


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
Stupid am I?  Stupid like a fox!

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[David C. <dl...@ha...>]

On Mon, Aug 11, 2003 at 06:29:26PM -0700, Michael G Schwern wrote:
> On Mon, Aug 11, 2003 at 03:18:10PM -0700, David Carmean wrote:
> > Just today I've created a builder subclass and have overridden the
> > test_files() method to look for $self->{args}{test_dirs} and add all the
> > *.t files found in or beneath those directories;  IMO much more convenient
> > than typing a long list for $self->{args}{test_files}.  So for instance
> > when I'm creating the Foo::Bar::Baz::Feh module in package Foo::Bar,
> > all the tests for Feh live in t/Foo/Bar/Baz/Feh, and I run "Build test
> > test_dirs=t/Foo/Bar/Baz/Feh".
> 
> The trouble with doing this by default is it makes this sort of testing
> very difficult:
> 
>     t/foo.t
>     t/Dummy-Module/lib/Dummy/Module.pm
>     t/Dummy-Module/t/Foo.t
> 
> where foo.t needs a little module to test with.  If you go recursive in
> scanning for .t files you'll pick up Foo.t.  There are workarounds but
> they're similar to yours above so you're just shifting the problem.
> 
> What might be better is a simple flag you can pass to Module::Build->new()
> which says "check recursively for tests".  Whether or not this should be
> the default is a matter to be settled by a steel cage match.


I was suggesting a new option, not a change to the default behavior: An 
argument named "test_dirs".  But unless I've missed a patch since 0.19 was 
posted to CPAN, M::B::Base::test_dirs already *does* recursively find ".t" files 
in t/ .

> > I'm trying to think of a reason not to have it add *.pl as well....?
> 
> The more things you look for as tests the harder it will be to have testing 
> data in t/.  Its desirable to keep the number of magical files to a minimum 
> else you magic it difficult to shake off that magic.
> 
> Since there's no compelling use-case for making *.pl magical other than "Look, 
> we can name it something else!" I'd say no.

It would just make vim recognize them as Perl files by default :o) .   Speaking of 
magic, where did the .t convention come from anway?

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Ken W. <ke...@ma...>]

On Tuesday, August 12, 2003, at 01:25  PM, David Carmean wrote:
>
> I was suggesting a new option, not a change to the default behavior: An
> argument named "test_dirs".  But unless I've missed a patch since 0.19 
> was
> posted to CPAN, M::B::Base::test_dirs already *does* recursively find 
> ".t" files
> in t/ .

Ooh, you're right!  I remember that setting off a flag in my head when 
I wrote it a long time ago, but then forgot about it.  It shouldn't 
scan recursively, for the reason Schwern mentions - as well as for 
compatibility reasons with MakeMaker layouts.

In some cases it might be handy to scan recursively though.  We'll have 
to think about a set of options that gives people flexibility in a few 
different ways.

> It would just make vim recognize them as Perl files by default :o) .   
> Speaking of magic, where did the .t convention come from anway?

It probably first appeared in the core, for the regression test scripts.

I agree with Schwern that the amount of magic in this area of M::B (and 
in most other areas) needs to stay very limited.  This can already be 
overridden by the user/subclasser anyway.  Plus, Test::Harness output 
doesn't/didn't look nice with .pl files.  PLUS, lots of people use .pl 
files to hold functions they share among their various test scripts.

  -Ken

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Steve P. <sp...@qu...>]

On Tuesday, August 12, 2003, at 08:31  pm, Ken Williams wrote:

> On Tuesday, August 12, 2003, at 01:25  PM, David Carmean wrote:
>>
>> I was suggesting a new option, not a change to the default behavior: 
>> An
>> argument named "test_dirs".  But unless I've missed a patch since 
>> 0.19 was
>> posted to CPAN, M::B::Base::test_dirs already *does* recursively find 
>> ".t" files
>> in t/ .
>
> Ooh, you're right!  I remember that setting off a flag in my head when 
> I wrote it a long time ago, but then forgot about it.  It shouldn't 
> scan recursively, for the reason Schwern mentions - as well as for 
> compatibility reasons with MakeMaker layouts.
>
> In some cases it might be handy to scan recursively though.  We'll 
> have to think about a set of options that gives people flexibility in 
> a few different ways.

A flag passed to new() would be a good idea, methinks.  Perhaps 
'test_recursive', 'rscan_tests' or 'scan_test_subdirs'?

Speaking of flexibility, a common request I've heard is to be able to 
run a subset of tests.  So what about letting the user run test scripts 
that match a regex:

	./Build tests "/.*foo.*/"
	./Build tests matching=".*foo.*"

or maybe shell-style wild cards:

	./Build tests "*foo*"

-Steve

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[David C. <dl...@ha...>]

On Tue, Aug 12, 2003 at 10:03:03PM +0100, Steve Purkis wrote:

> A flag passed to new() would be a good idea, methinks.  Perhaps 
> 'test_recursive', 'rscan_tests' or 'scan_test_subdirs'?
> 
> Speaking of flexibility, a common request I've heard is to be able to 
> run a subset of tests.  So what about letting the user run test scripts 
> that match a regex:
> 
> 	./Build tests "/.*foo.*/"
> 	./Build tests matching=".*foo.*"

Easily running subsets of tests is exactly what prompted me to start this 
discussion.  With my test_dirs hack to my subclassed builders, I accomplish 
this by making stubs off of a directory tree.  For example, if I have the following 
class heirarchy in my package:

    Foo::Bar
    Foo::Bar::One
    Foo::Bar::One::A
    Foo::Bar::One::B

Then tests for Foo::Bar::One::A live in t/Foo/Bar/One/A/t, 
tests for private methods of Foo::Bar::One live in t/Foo/Bar/One/t, 
and high-level tests for the entire distribution live in t/Foo/Bar.

If I want to test only the methods in the parent class Foo::Bar::One, 
I run "Build test test_dirs=t/Foo/Bar/One/t".   If I want to run 
the tests for that class and all it's children, I run 
"Build test test_dirs=t/Foo/Bar/One".  If I'm fixing a bug in Foo::Bar::One::B, 
I run "Build test test_dirs=t/Foo/Bar/One/B/t" over and over, and then when I'm 
satisfied I run a full regression test with "Build test".

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Steve P. <sp...@qu...>]

On Tuesday, August 12, 2003, at 10:35  pm, David Carmean wrote:

> If I want to test only the methods in the parent class Foo::Bar::One,
> I run "Build test test_dirs=t/Foo/Bar/One/t".   If I want to run
> the tests for that class and all it's children, I run
> "Build test test_dirs=t/Foo/Bar/One".  If I'm fixing a bug in 
> Foo::Bar::One::B,
> I run "Build test test_dirs=t/Foo/Bar/One/B/t" over and over, and then 
> when I'm
> satisfied I run a full regression test with "Build test".

That works fine if you organize tests by directory, but a lot of 
authors like to group their tests by function in the /t directory - 
M::B's suite is a good example of that:

	t/basic.t
	t/compat.t
	t/runthrough.t
	...

And I know a lot of people (myself included) who also like to number 
them according to priority.  So for example, you might see something 
like:

	t/01..basic.t
	t/02..framework.t
	t/03..foo-bar.t
	...

Using regexps on the path to the test script would let you specify a 
subset of tests to run in all of these scenarios:

	./Build test matching=t/Foo/Bar/One
	./Build test matching=runthrough
	./Build test matching=t/0*

Which could be handy.

-Steve

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Michael G S. <sc...@po...>]

On Tue, Aug 12, 2003 at 11:51:16PM +0100, Steve Purkis wrote:
> >If I want to test only the methods in the parent class Foo::Bar::One,
> >I run "Build test test_dirs=t/Foo/Bar/One/t".   If I want to run
> >the tests for that class and all it's children, I run
> >"Build test test_dirs=t/Foo/Bar/One".  If I'm fixing a bug in 
> >Foo::Bar::One::B,
> >I run "Build test test_dirs=t/Foo/Bar/One/B/t" over and over, and then 
> >when I'm
> >satisfied I run a full regression test with "Build test".
> 
> That works fine if you organize tests by directory, but a lot of 
> authors like to group their tests by function in the /t directory - 

This may be a habit formed from MakeMaker's limitation on directory
depth.


> Using regexps on the path to the test script would let you specify a 
> subset of tests to run in all of these scenarios:
> 
> 	./Build test matching=t/Foo/Bar/One
> 	./Build test matching=runthrough
> 	./Build test matching=t/0*
> 
> Which could be handy.

I presume you mean globs, not regexes?

What might be a lot simpler is semantics like this:

    # run only foo.t
    Build test test_files=t/foo.t

    # run all .t files in t/
    Build test test_files=t/*.t

so rather than having "test_files" and "matching" as seperate beasts, just
run the test_files arguments through glob().

Additionally, one can add this shorthand.

    # equivalent to t/*.t
    build test test_files=t/

This is handy because if/when we get the "look for test files recursively"
option, the above would go recursive.  Otherwise there's no easy
way to say "test everything in this directory and what's below it".

This patch implements both.

--- lib/Module/Build/Base.pm	2003/08/12 23:53:00	1.2
+++ lib/Module/Build/Base.pm	2003/08/12 23:58:27
@@ -875,13 +875,27 @@
   
   my @tests;
   if ($self->{args}{test_files}) {  # XXX use 'properties'
-    @tests = $self->split_like_shell($self->{args}{test_files});
+    @tests = map { glob $_ }
+             map { -d $_ ? $self->expand_test_dir($_) : $_ }
+                 $self->split_like_shell($self->{args}{test_files});
   } else {
     # Find all possible tests in t/ or test.pl
     push @tests, 'test.pl'                          if -e 'test.pl';
     push @tests, @{$self->rscan_dir('t', qr{\.t$})} if -e 't' and -d _;
   }
   return [sort @tests];
+}
+
+sub expand_test_dir {
+    my($self, $dir) = @_;
+
+    opendir(DIR, $dir) or die "Can't open $dir: $!";
+    my @tests = map  { File::Spec->catfile($dir, $_) }
+                grep { /\.t$/ } 
+                     readdir DIR;
+    closedir DIR;
+
+    return @tests;
 }
 
 sub ACTION_testdb {


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
Do not try comedy at home!  Milk & Cheese are advanced experts!  Attempts at
comedy can be dangerously unfunny!

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Steve P. <sp...@qu...>]

On Wednesday, August 13, 2003, at 01:02  am, Michael G Schwern wrote:

> On Tue, Aug 12, 2003 at 11:51:16PM +0100, Steve Purkis wrote:
>>> If I want to test only the methods in the parent class Foo::Bar::One,
>>> I run "Build test test_dirs=t/Foo/Bar/One/t".   If I want to run
>>> the tests for that class and all it's children, I run
>>> "Build test test_dirs=t/Foo/Bar/One".  If I'm fixing a bug in
>>> Foo::Bar::One::B,
>>> I run "Build test test_dirs=t/Foo/Bar/One/B/t" over and over, and 
>>> then
>>> when I'm
>>> satisfied I run a full regression test with "Build test".
>>
>> That works fine if you organize tests by directory, but a lot of
>> authors like to group their tests by function in the /t directory -
>
> This may be a habit formed from MakeMaker's limitation on directory
> depth.

Maybe, but it's also something that's not going to go away anytime 
soon.  And I'm not convinced it's a bad habit (especially not for small 
projects).


>> Using regexps on the path to the test script would let you specify a
>> subset of tests to run in all of these scenarios:
>>
>> 	./Build test matching=t/Foo/Bar/One
>> 	./Build test matching=runthrough
>> 	./Build test matching=t/0*
>>
>> Which could be handy.
>
> I presume you mean globs, not regexes?

Nope, I meant regexes, that's just a typo in my example:

	./Build test matching=t/0.*

Here's some better examples:

	# run all tests found that have 'foo' in their names:
	./Build test matching=Foo

	# for David's example, same as this (recursive) glob:
	./Build test test_files=t/Foo/

	# ... but might also include:
	./Build test test_files=t/Bar/Foo/

Us numbered-test freaks could run sets of tests like this:

	# run tests 50-99:
	./Build test matching=[5-9]\\d

And 'matching' can co-exist with globs, which are also a good idea:

	# run all tests in t/Foo ending in "oo.t"
	./Build test test_files=t/Foo/* matching=oo\\.t

I've attached a patch based on your glob one that implements this.

-Steve

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Ken W. <ke...@ma...>]

On Tuesday, August 12, 2003, at 07:02  PM, Michael G Schwern wrote:
> What might be a lot simpler is semantics like this:
>
>     # run only foo.t
>     Build test test_files=t/foo.t
>
>     # run all .t files in t/
>     Build test test_files=t/*.t
>
> so rather than having "test_files" and "matching" as seperate beasts, 
> just
> run the test_files arguments through glob().

Okay, here's what I'll do.  I like running 'test_files' through glob(), 
for several reasons.  First, it means the default changes from a bunch 
of code that finds the files, to the string "t/*.t".  Second, I like 
the ease of specifying sets of stuff this way, and I think it scales 
well.

I think the semantics should work like this:

  * Run through split_like_shell().

  * Expand with glob().

  * Pass through files unaltered, and expand directories using the likes 
of glob("$_/*.t").

This will match people's expectations from the shell a little more 
closely, I think.

Here's how I'll apply the first patch.  The 'recursive_test_files' 
property remains unimplemented and I'm not sure about the name yet.  
Steve's 'matching' argument can still be implemented later if we want 
regexes.

  -Ken

--- lib/Module/Build/Base.pm    12 Aug 2003 15:52:39 -0000      1.165
+++ lib/Module/Build/Base.pm    13 Aug 2003 15:24:25 -0000
@@ -1012,13 +1012,21 @@

    my @tests;
    if ($self->{args}{test_files}) {  # XXX use 'properties'
-    @tests = $self->split_like_shell($self->{args}{test_files});
+    @tests = (map { -d $_ ? $self->expand_test_dir($_) : $_ }
+             map glob,
+             $self->split_like_shell($self->{args}{test_files}));
    } else {
      # Find all possible tests in t/ or test.pl
      push @tests, 'test.pl'                          if -e 'test.pl';
-    push @tests, @{$self->rscan_dir('t', qr{\.t$})} if -e 't' and -d _;
+    push @tests, $self->expand_test_dir('t')        if -e 't' and -d _;
    }
    return [sort @tests];
+}
+
+sub expand_test_dir {
+  my ($self, $dir) = @_;
+  return @{$self->rscan_dir($dir, qr{\.t$})} if 
$self->{properties}{recursive_test_files};
+  return glob File::Spec->catfile($dir, "*.t");
  }

  sub ACTION_testdb {

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Ken W. <ke...@ma...>]

On Wednesday, August 13, 2003, at 10:26  AM, Ken Williams wrote:
> Here's how I'll apply the first patch.  The 'recursive_test_files' 
> property remains unimplemented and I'm not sure about the name yet.  
> Steve's 'matching' argument can still be implemented later if we want 
> regexes.

I also just made test_files() a read/write accessor, so that I could 
write a proper test case for the globbing.

Can anyone comment on the cross-platform abilities of glob()?

  -Ken

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Michael G S. <sc...@po...>]

On Wed, Aug 13, 2003 at 10:26:35AM -0500, Ken Williams wrote:
> Okay, here's what I'll do.  I like running 'test_files' through glob(), 
> for several reasons.  First, it means the default changes from a bunch 
> of code that finds the files, to the string "t/*.t".

Danger!  Sorting order.  Using glob("t/*.t") as the default order means
it is no longer guaranteed.  At best they'll be ASCIIbetical, but even that 
depends on the glob implementation.


> -    push @tests, @{$self->rscan_dir('t', qr{\.t$})} if -e 't' and -d _;
> +    push @tests, $self->expand_test_dir('t')        if -e 't' and -d _;

just change that to

    push @tests, sort { lc $a cmp lc $b } $self->expand_test_dir('t') ...

in order to guarantee that "Build test" runs in the same order on every OS.
Don't sort anything but the default, though.  You want to leave an opening
for users to change the order.


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
Don't step on my funk

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Ken W. <ke...@ma...>]

On Wednesday, August 13, 2003, at 03:02  PM, Michael G Schwern wrote:
> Danger!  Sorting order.  Using glob("t/*.t") as the default order means
> it is no longer guaranteed.  At best they'll be ASCIIbetical, but even 
> that
> depends on the glob implementation.

Yeah, but they do get sorted later on.

> just change that to
>
>     push @tests, sort { lc $a cmp lc $b } $self->expand_test_dir('t') 
> ...
>
> in order to guarantee that "Build test" runs in the same order on 
> every OS.
> Don't sort anything but the default, though.  You want to leave an 
> opening
> for users to change the order.

I think the sorting should move from where it is now (end of 
test_files()) to inside the expand_test_dir() routine.  That will 
preserve any order the user specifies with the 'test_files' parameter, 
while giving predictable sorting order on glob expansions.

  -Ken

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Michael G S. <sc...@po...>]

On Wed, Aug 13, 2003 at 03:58:28PM -0500, Ken Williams wrote:
> I think the sorting should move from where it is now (end of 
> test_files()) to inside the expand_test_dir() routine.  That will 
> preserve any order the user specifies with the 'test_files' parameter, 
> while giving predictable sorting order on glob expansions.

Sensible.


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
Thanks, applied.  Or, ??????k@???????K^U  
as we Bulgarian EBCDIC users like to say.    
	-- Jarkko Hietaniemi in <200...@al...>

Re: [Module-build-general] RFC: Build test test_dirs=(list)

[Michael G S. <sc...@po...>]

On Tue, Aug 12, 2003 at 11:25:14AM -0700, David Carmean wrote:
> It would just make vim recognize them as Perl files by default :o).   
> Speaking of magic, where did the .t convention come from anway?

Most of the Deep Testing Conventions date back to perl1.


-- 
Michael G Schwern        sc...@po...  http://www.pobox.com/~schwern/
It's Ecstacy time!