Mailing List Archive

On Feb 4, 2010, at 6:13 AM, Mark Jaroski wrote:

> OK, so that's done. I also sketched out that idea that I mentioned about
> debugging. Have a look here:
>
> http://github.com/MarkJaroski/bricolage/commit/6004916c9a187778568432ba8514317d9fe78238

The commit message says, "Switched to transactional approch to setup and teardown." But it sure looks more like you just disable all database access. That's not desirable. The database access must be tested, too. Also, you need to delete the cached mocks in a teardown method, otherwise they'll last for all tests after that one runs (the test object is a singleton).

> and here:
>
> http://github.com/MarkJaroski/bricolage/commit/600fdc9f04480083c5abfa0555e60a68f19d88cc

Yuck. I'm against having test-specific code in modules. Also, why did you remove some of the calls to test functions?

- ok( my $burner = Bric::Util::Burner->new, "Get burner" );
- ok( $burner->deploy($tmpl[0]), "Deploy autohandler template" );
- ok( $burner->deploy($tmpl[1]), "Deploy story template" );
+ my $burner = Bric::Util::Burner->new;
+ $burner->deploy($tmpl[0]);
+ $burner->deploy($tmpl[1]);

Those methods return true values. Please put them back.

Best,

David

David E. Wheeler wrote:
> On Feb 4, 2010, at 6:13 AM, Mark Jaroski wrote:
>
> > OK, so that's done. I also sketched out that idea that I mentioned about
> > debugging. Have a look here:
> >
> > http://github.com/MarkJaroski/bricolage/commit/6004916c9a187778568432ba8514317d9fe78238
>
> The commit message says, "Switched to transactional approch to setup and
> teardown." But it sure looks more like you just disable all database
> access. That's not desirable. The database access must be tested, too.
> Also, you need to delete the cached mocks in a teardown method, otherwise
> they'll last for all tests after that one runs (the test object is a
> singleton).

Maybe we're looking at two different things, but as far as I can tell I've
only done as you suggested.


> > and here:
> >
> > http://github.com/MarkJaroski/bricolage/commit/600fdc9f04480083c5abfa0555e60a68f19d88cc
>
> Yuck. I'm against having test-specific code in modules. Also, why did you remove some of the calls to test functions?
>
> - ok( my $burner = Bric::Util::Burner->new, "Get burner" );
> - ok( $burner->deploy($tmpl[0]), "Deploy autohandler template" );
> - ok( $burner->deploy($tmpl[1]), "Deploy story template" );
> + my $burner = Bric::Util::Burner->new;
> + $burner->deploy($tmpl[0]);
> + $burner->deploy($tmpl[1]);
>
> Those methods return true values. Please put them back.

I'm not keen on testing stuff that's not in the SUT. As far as I can tell
that stuff is tested elsewhere. The only thing testing it again here does
is to inflate the number of tests without really testing anything new. That
runs against best practices in x-unit testing.

--
--
=================================================================
-- mark at geekhive dot net --

On Feb 4, 2010, at 4:35 PM, Mark wrote:

> Maybe we're looking at two different things, but as far as I can tell I've
> only done as you suggested.

No, I suggested switching to transactions to manage rolling back changes. You suggested mocking out all the database stuff so as to test how SQL queries are generated. This change does neither: it simply turns off the database access.

If you want to *add* tests that don't touch the database, in order to test SQL generation, that's fine. But please don't change tests such that they end up not testing something they did before.

>> Those methods return true values. Please put them back.
>
> I'm not keen on testing stuff that's not in the SUT. As far as I can tell
> that stuff is tested elsewhere. The only thing testing it again here does
> is to inflate the number of tests without really testing anything new. That
> runs against best practices in x-unit testing.

1. WTF is SUT?
2. Where is that stuff tested elsewhere.
3. I don't give a fuck. Add new tests if you like, but please do *not* delete existing ones unless they functionality they change is going away.

Best,

David

____________________________________
From: David E. Wheeler
Sent: Thu, Feb 04, 2010 at 06:04:55PM +0100
To: To devel@lists.bricolage.cc
Subject: Re: Job Manager Improvements

> > Of course to do this would require building lots of mock objects on the
> > fly, so it would be a major gigantic test re-factoring. Just to be able to
> > run tests without a db it's not worth it.
>
> You'd just need to mock a few of the functions in Bric::Util::DBI.

But then for each test I'd have to take the output from those mocked
functions and check that it contains the correct SQL, and the correct
arguments.

Then in order to test that the SQL actually works there'd have to be some
more tests which connect to the db and actually run the SQL which I'd
tested for.

So really in order to exercise everything it would be a very serious
effort.


--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................
Always remember: unencrypted email is not private. Be careful.
.................................................................

____________________________________
From: David E. Wheeler
Sent: Fri, Feb 05, 2010 at 03:45:34AM +0100
To: To devel@lists.bricolage.cc
Subject: Re: Job Manager Improvements

> On Feb 4, 2010, at 4:35 PM, Mark wrote:
>
> > Maybe we're looking at two different things, but as far as I can tell I've
> > only done as you suggested.
>
> No, I suggested switching to transactions to manage rolling back changes.

Yes.

> You suggested mocking out all the database stuff so as to test how SQL
> queries are generated.

My idea about mocking the whole db was just hypothetical. It would be a
lot of work to do that.

> This change does neither: it simply turns off the database access.

I'm pretty much certain that it doesn't do that. When I add a call to the
method for dumping database to the teardown, before the call to rollback it
produces output showing that there are the expected rows in the tables
(attached).

Besides, the tests wouldn't pass otherwise, and they do.

> If you want to *add* tests that don't touch the database, in order to
> test SQL generation, that's fine. But please don't change tests such that
> they end up not testing something they did before.

OK.

> >> Those methods return true values. Please put them back.

OK.

> > I'm not keen on testing stuff that's not in the SUT. As far as I can tell
> > that stuff is tested elsewhere. The only thing testing it again here does
> > is to inflate the number of tests without really testing anything new. That
> > runs against best practices in x-unit testing.
>
> 1. WTF is SUT?

System Under Test: http://xunitpatterns.com/SUT.html

> 2. Where is that stuff tested elsewhere.

t/Bric/Util/Burner/Template/DevTest.pm


> 3. I don't give a fuck. Add new tests if you like, but please do *not*
> delete existing ones unless they functionality they change is going away.

OK. No problem. I'll put the tests back.

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................
Always remember: unencrypted email is not private. Be careful.
.................................................................

____________________________________
From: David E. Wheeler
Sent: Thu, Feb 04, 2010 at 06:10:18PM +0100
To: To devel@lists.bricolage.cc
Subject: Re: Job Manager Improvements

> Yuck. I'm against having test-specific code in modules.

I totally agree, but in this case it seems pretty harmless. It's just
replacing an un-mockable constant with a mockable subroutine. Or is the
constant actually mockable?

Is there a better way to do this that I'm not seeing?

thanks again,

-mark

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................
Always remember: unencrypted email is not private. Be careful.
.................................................................

David E. Wheeler wrote:
> 3. I don't give a fuck. Add new tests if you like, but please do *not*
> delete existing ones unless they functionality they change is going away.

http://github.com/MarkJaroski/bricolage/commit/30827ef1b8d788e133203f817b1b59f0654fae28


--
--
=================================================================
-- mark at geekhive dot net --

On Feb 5, 2010, at 1:17 AM, Mark Jaroski wrote:

> My idea about mocking the whole db was just hypothetical. It would be a
> lot of work to do that.

And not worth it, IMHO. I thought it was kind of insane.

> I'm pretty much certain that it doesn't do that. When I add a call to the
> method for dumping database to the teardown, before the call to rollback it
> produces output showing that there are the expected rows in the tables
> (attached).
>
> Besides, the tests wouldn't pass otherwise, and they do.

Then what's with all the mocks? If you're using transactions and rolling back at the end, the mocks shouldn't be needed at all.

http://github.com/MarkJaroski/bricolage/commit/6004916c9a187778568432ba8514317d9fe78238

> OK. No problem. I'll put the tests back.

Thanks,

David

On Feb 5, 2010, at 4:31 AM, Mark Jaroski wrote:

> I totally agree, but in this case it seems pretty harmless. It's just
> replacing an un-mockable constant with a mockable subroutine. Or is the
> constant actually mockable?
>
> Is there a better way to do this that I'm not seeing?

Why do you need to turn on debugging?

David

____________________________________
From: David E. Wheeler
Sent: Sun, Feb 07, 2010 at 12:26:23AM +0100
To: To devel@lists.bricolage.cc
Subject: Re: Job Manager Improvements

> Why do you need to turn on debugging?

Presumably the code will not always be perfect, so sometimes tests will
fail. When that happens I find myself turning on DBI_DEBUG, and slogging
through the pages of output that it produces.

It would be better to be able to do isolated debugging for the failing test
case, so when you get a test case failing you can do this (pseudo-code):

ok( $obj->do_something, "Tried to do_something to obj: "
. $obj->get_id
. debug_on . $obj->do_something . debug_off
. dump_debug_data );


Or similar. I figure the more information you can get out of the failing
test the better, because the state at the time of failure is what you want
to look at.

Otherwise it's really hard to isolate that state in the exact moment, be it
with breakpoints or whatever.

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................
Always remember: unencrypted email is not private. Be careful.
.................................................................

____________________________________
From: David E. Wheeler
Sent: Sun, Feb 07, 2010 at 12:25:37AM +0100
To: To devel@lists.bricolage.cc
Subject: Re: Job Manager Improvements

> On Feb 5, 2010, at 1:17 AM, Mark Jaroski wrote:
>
> Then what's with all the mocks? If you're using transactions and rolling back at the end, the mocks shouldn't be needed at all.

A wise man told me I could do it that way:

>> begin;
>> $job->lock($whoiam); # or maybe $job->claim($whoiam);
>> commit;
>>
>> begin;
>> $job->execute;
>> commit;
>
>No. Bad idea. But you *can* use Test::MockModule to replaces the begin and
>commit functions at runtime, even put tests in them to make sure that the
>jobs call them a the right +times.

Once I had groked how Test::MockModule works I realized that it was a
really clean solution. It allows you to override just one subroutine or
method out of a module, and only for the lifetime of of the test harness,
which is exactly what you want for testing.

Obviously I still have to put the tests in to make sure that commit and
begin are being called at the right times.

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................
Always remember: unencrypted email is not private. Be careful.
.................................................................

On Feb 8, 2010, at 12:53 AM, Mark Jaroski wrote:

>> Then what's with all the mocks? If you're using transactions and rolling back at the end, the mocks shouldn't be needed at all.
>
> A wise man told me I could do it that way:

Oh, I see. Quite right, although the scope is still wrong.

>> No. Bad idea. But you *can* use Test::MockModule to replaces the begin and
>> commit functions at runtime, even put tests in them to make sure that the
>> jobs call them a the right +times.
>
> Once I had groked how Test::MockModule works I realized that it was a
> really clean solution.

Yes. I use it a *lot*.

> It allows you to override just one subroutine or
> method out of a module, and only for the lifetime of of the test harness,
> which is exactly what you want for testing.

No, actually, you usually just want it for the duration of a single test method. Never for the duration of all tests run by the harness.

> Obviously I still have to put the tests in to make sure that commit and
> begin are being called at the right times.

Right, I missed that, sorry.

Best,

David

On Feb 8, 2010, at 12:47 AM, Mark Jaroski wrote:

> Or similar. I figure the more information you can get out of the failing
> test the better, because the state at the time of failure is what you want
> to look at.
>
> Otherwise it's really hard to isolate that state in the exact moment, be it
> with breakpoints or whatever.

/me shrugs. I dunno, I just turned it on and ran the one test file, or put a print statement in the code to see the SQL. Or turned on `log_statement` in postgresql.conf and tailed the PostgreSQL log.

Best,

David

David E. Wheeler wrote:
> No, actually, you usually just want it for the duration of a single test
> method. Never for the duration of all tests run by the harness.

The harness only exists for the duration of a single test method. The
setup and teardown methods are called before and after each test method.

--
--
=================================================================
-- mark at geekhive dot net --

On Feb 8, 2010, at 12:32 PM, Mark wrote:

>> No, actually, you usually just want it for the duration of a single test
>> method. Never for the duration of all tests run by the harness.
>
> The harness only exists for the duration of a single test method. The
> setup and teardown methods are called before and after each test method.

Okay, your use of terms is confusing. A "harness" is a program that collects TAP and analyzes it. It's completely separate from the tests themselves. A Test::Class object (or a subclass, as in our case), is passed as the first argument to our methods (indeed, these are instance methods).

Now, the setup and teardown methods are called before and after each test method in a class (and before each test method in any subclasses). This is true. And if you want to scope mocked methods/functions to an entire test class and its subclasses, then yes you can mock them out in setup() and do nothing else. However, for documentation and sanity purposes, if nothing else, you should remove mocks in teardown methods, so that the the scope is explicit.

But more importantly, I think that such mocks should *not* be in the setup methods. Better is to add tests to the individual test methods to make sure that begin/commit/rollback are called in the appropriate places. Maybe there's a way to generalize that for all methods in a class by mocking them in setup(). That'd be fine.

But otherwise, mocks should be close to where they're required, and their scope much more explicit when they're not.

Best,

David

David E. Wheeler wrote:
> Okay, your use of terms is confusing. A "harness" is a program that
> collects TAP and analyzes it.

Sorry. Slip of the keyboard. I meant "fixture". I have no idea why I wrote
harness instead. I'd like to assure you that I know the difference, but I
suppose this error makes me sound like I don't, which is unfortunate.

> Now, the setup and teardown methods are called before and after each test
> method in a class (and before each test method in any subclasses). This
> is true. And if you want to scope mocked methods/functions to an entire
> test class and its subclasses, then yes you can mock them out in setup()
> and do nothing else. However, for documentation and sanity purposes, if
> nothing else, you should remove mocks in teardown methods, so that the
> the scope is explicit.

OK. I can do that. I'd put them in the setup for convenience, since begin
and commit have to be turned off everywhere in order for the tests to work
without either saving objects to delete or running a Pg_reset.sql type
script in each tear-down.

> But more importantly, I think that such mocks should *not* be in the
> setup methods.

They certainly are easier to get at in the test methods.

> Better is to add tests to the individual test methods to make sure that
> begin/commit/rollback are called in the appropriate places. Maybe there's
> a way to generalize that for all methods in a class by mocking them in
> setup(). That'd be fine.
>
> But otherwise, mocks should be close to where they're required, and their
> scope much more explicit when they're not.

OK. Thanks!

--
--
=================================================================
-- mark at geekhive dot net --

David E. Wheeler wrote:
> /me shrugs. I dunno, I just turned it on and ran the one test file, or
> put a print statement in the code to see the SQL. Or turned on
> `log_statement` in postgresql.conf and tailed the PostgreSQL log.

Yeah, that's how I was doing it too. But, it takes a lot of effort that way
to get at the particular state that you need, so it seems nice to have to
option to turn it on in the bit of code (the test) where you know you have
the state that you want to look at.

--
--
=================================================================
-- mark at geekhive dot net --

On Feb 8, 2010, at 9:59 PM, Mark wrote:

>> Okay, your use of terms is confusing. A "harness" is a program that
>> collects TAP and analyzes it.
>
> Sorry. Slip of the keyboard. I meant "fixture". I have no idea why I wrote
> harness instead. I'd like to assure you that I know the difference, but I
> suppose this error makes me sound like I don't, which is unfortunate.

Heh, shit happens. Funny thing is, I don't think of the test data as fixtures, but just test data. That's because they're not loaded from a separate file or anything, just created within the tests themselves. I suppose that's an arbitrary distinction, one of the ways in which my 18 months or Rails development atez my brainz.

> OK. I can do that. I'd put them in the setup for convenience, since begin
> and commit have to be turned off everywhere in order for the tests to work
> without either saving objects to delete or running a Pg_reset.sql type
> script in each tear-down.

Yes, if you're turning off transactions for all the methods in a test class and its subclasses, then it makes sense to disable transaction functions from there, too. As long as that's reversed in the teardown, it'll feel a bit more sane to me, because the scoping will be explicit.

>> But more importantly, I think that such mocks should *not* be in the
>> setup methods.
>
> They certainly are easier to get at in the test methods.

Yes.

Best,

David

David E. Wheeler wrote:
> On Feb 8, 2010, at 9:59 PM, Mark wrote:
> > OK. I can do that. I'd put them in the setup for convenience, since begin
> > and commit have to be turned off everywhere in order for the tests to work
> > without either saving objects to delete or running a Pg_reset.sql type
> > script in each tear-down.
>
> Yes, if you're turning off transactions for all the
> methods in a test class and its subclasses, then it makes
> sense to disable transaction functions from there, too. As
> long as that's reversed in the teardown, it'll feel a bit
> more sane to me, because the scoping will be explicit.

I've done that, and added 36 new tests for searching for
jobs by URI:

http://github.com/MarkJaroski/bricolage/commit/614d29229c0dcfbf705d085eb600680334cf5103

> >> But more importantly, I think that such mocks should *not* be in the
> >> setup methods.
> >
> > They certainly are easier to get at in the test methods.
>
> Yes.

I've also written up the strategy for testing list by user
and locking. I'll be mocking those subroutines in the test
for the later:

http://github.com/MarkJaroski/bricolage/commit/363d81c3e27bd42ce5f58dd496237048fd159e38

--
--
=================================================================
-- mark at geekhive dot net --

On Feb 10, 2010, at 12:15 AM, Mark wrote:

> I've done that, and added 36 new tests for searching for
> jobs by URI:
>
> http://github.com/MarkJaroski/bricolage/commit/614d29229c0dcfbf705d085eb600680334cf5103

Excellent, thanks.

> I've also written up the strategy for testing list by user
> and locking. I'll be mocking those subroutines in the test
> for the later:
>
> http://github.com/MarkJaroski/bricolage/commit/363d81c3e27bd42ce5f58dd496237048fd159e38

Cool, thanks. Please do let us know when you feel that this stuff is ready to be merged, and after 2.0 drops and we open up 2.1, I'll have a look at the overall patch again.

Best,

David

David E. Wheeler wrote:
> Cool, thanks. Please do let us know when you feel that this stuff is
> ready to be merged, and after 2.0 drops and we open up 2.1, I'll have a
> look at the overall patch again.

I've added the rest of those tests. Everything passes. So, I guess it's
pretty much ready to go.

I still have that debugging subroutine in DBI, but really if you think
about it that's not really test code, but rather debugging code, which
simply replaces slightly different debugging code.

Anyhow take your time. No worries.

The next thing I need to look at in Bricolage is the element XML stuff,
unless you've already re-written it. We make pretty heavy use of it, and
were getting some errors after our upgrade, so I made some quick hackish
fixes which I need to clean up and send to you.

thanks,

-mark

--
--
=================================================================
-- mark at geekhive dot net --

On Feb 10, 2010, at 12:26 PM, Mark wrote:

> I still have that debugging subroutine in DBI, but really if you think
> about it that's not really test code, but rather debugging code, which
> simply replaces slightly different debugging code.

I think I'd rather see something like this:

use constant DEBUG => DBI_DEBUG || $ENV{BRIC_DBI_DEBUG} || 0;

That way you can get what you nee without effecting the runtime.

Best,

David