Mailing List Archive: Bricolage: devel

Job Manager Improvements

1 2

View All

Index | Next | Previous | View Threaded

jaroskim at who

Sep 2, 2009, 7:23 AM

Post #1 of 47 (3977 views)
Permalink

Job Manager Improvements

Hi there,

I've been filling in for the person who normally does Bricolage development
here at WHO for the last couple of months, mainly doing maintenance tasks.

One of the things we've been working on is a quality improvement program
for the public website, which includes finding some ways to learn more
about how our internal users are using Bricolage.

Since I already know a little bit about the Job queue it made sense to
start out by making some improvement to the Job manager, so that our web
team members can find out more about not just which publishing and
distribution jobs are scheduled, but also about jobs which have been
completed. The team has also expressed a desire to be able to search by
the URI of a story being published, or by its name, rather than that of the
job.

So anyhow, I spent some time relearning the Bricolage code-base, and coding
this change up. There are changes in two places, in the API layer and in
the UI layer.

So my question is this. Would you rather have the whole thing as one big
patch, or would you rather see the API patch first and then the UI layer
patch? I think the UI layer patch is going to need the most work before it
can be committed, and of course it depends on the API layer, so that will
have to go in first (if at all) anyhow.

Thanks!

-mark

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................

Attachments:

smime.p7s (2.17 KB)

david at kineticode

Sep 2, 2009, 9:17 AM

Post #2 of 47 (3871 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Sep 2, 2009, at 7:23 AM, Mark Jaroski wrote:

> So anyhow, I spent some time relearning the Bricolage code-base, and
> coding
> this change up. There are changes in two places, in the API layer
> and in
> the UI layer.

Not in callbacks?

> So my question is this. Would you rather have the whole thing as
> one big
> patch, or would you rather see the API patch first and then the UI
> layer
> patch? I think the UI layer patch is going to need the most work
> before it
> can be committed, and of course it depends on the API layer, so that
> will
> have to go in first (if at all) anyhow.

Send the whole thing. If it's too much, I'll ask you to break it up.
But I'd like to get a feel for the whole thing.

Better yet, fork and commit to a branch on GitHub. Then I can just
check it out.

Best,

David

jaroskim at who

Sep 3, 2009, 12:06 AM

Post #3 of 47 (3873 views)
Permalink

Re: Job Manager Improvements [In reply to]

____________________________________
From: David E. Wheeler
Sent: Wed, Sep 02, 2009 at 06:17:02PM +0200
To: To devel [at] lists
Subject: Re: Job Manager Improvements

> On Sep 2, 2009, at 7:23 AM, Mark Jaroski wrote:
>
> > So anyhow, I spent some time relearning the Bricolage code-base, and
> > coding
> > this change up. There are changes in two places, in the API layer
> > and in
> > the UI layer.
>
> Not in callbacks?

That would be ideal, but I'm not very confident with the callback
architecture yet so I just made a new dhandler for it.

> > So my question is this. Would you rather have the whole thing as
> > one big
> > patch, or would you rather see the API patch first and then the UI
> > layer
> > patch? I think the UI layer patch is going to need the most work
> > before it
> > can be committed, and of course it depends on the API layer, so that
> > will
> > have to go in first (if at all) anyhow.
>
> Send the whole thing. If it's too much, I'll ask you to break it up.
> But I'd like to get a feel for the whole thing.
>
> Better yet, fork and commit to a branch on GitHub. Then I can just
> check it out.

OK.

thanks,

-mark

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................

Attachments:

smime.p7s (2.17 KB)

jaroskim at who

Sep 8, 2009, 6:44 AM

Post #4 of 47 (3835 views)
Permalink

Re: Job Manager Improvements [In reply to]

____________________________________
From: David E. Wheeler
Sent: Wed, Sep 02, 2009 at 06:17:02PM +0200
To: To devel [at] lists
Subject: Re: Job Manager Improvements
>
> Not in callbacks?

Happily, but I'll need coaching on how you want callbacks done.

> Better yet, fork and commit to a branch on GitHub. Then I can just
> check it out.

http://github.com/MarkJaroski/bricolage/tree/dev_job_fancy_queue

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................

Attachments:

smime.p7s (2.17 KB)

david at kineticode

Oct 5, 2009, 5:15 PM

Post #5 of 47 (3540 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Sep 8, 2009, at 6:44 AM, Mark Jaroski wrote:

>> Not in callbacks?
>
> Happily, but I'll need coaching on how you want callbacks done.
>
>> Better yet, fork and commit to a branch on GitHub. Then I can just
>> check it out.
>
> http://github.com/MarkJaroski/bricolage/tree/dev_job_fancy_queue

Sorry for the delay. Just one commit so far? Doesn't look too long,
but can you give me a high-level overview of what you've changed and
why?

Thanks,

David

jaroskim at who

Oct 6, 2009, 5:31 AM

Post #6 of 47 (3551 views)
Permalink

Re: Job Manager Improvements [In reply to]

____________________________________
From: David E. Wheeler
Sent: Tue, Oct 06, 2009 at 02:15:10AM +0200
To: To devel [at] lists
Subject: Re: Job Manager Improvements
> >
> > http://github.com/MarkJaroski/bricolage/tree/dev_job_fancy_queue
>
> Sorry for the delay. Just one commit so far?

Yes, I had it ready to submit as a patch to the list, so I pulled a new
version of the code, branched it, and applied my patch. The patch applied
cleanly so I committed the branch.

> Doesn't look too long, but can you give me a high-level overview of what
> you've changed and why?

What:

The Job class now supports searching by attributes of the assets associated
with a publish job. In order to do this cleanly I had to use a LEFT JOIN
with the media_url and story_url tables. This in turn suggested re-writing
the some of the other joins as explicit joins for consistency.

While I was at it I removed the table aliases to conform with the current
coding standard.

I'm less confident that you'll accept the UI layer changes, because I did
everything in the mason components rather than trying to get my head around
the callback system. I'll need some coaching to do that.

The changes we made to the UI are below, but a few more words on the
implementation first: I basically stripped all of the job manager code out
of the generic dhandler, and then created a specific dhandler for jobs.
All of the code implementing the changes in the job UI are isolated there.

Why:

The goal of the UI changes is to make it easier for editors of a very large
Bricolage installation to watch what the other editors are doing, mainly
for quality control. This is particularly necessary in our organization
since we have a very large number of editors from disparate departments who
don't always follow the standards set by the central web-team. The web
team can't handle the additional workload of being part of the workflow for
these units, however they do need to know in general what is getting
published, so they can spot-check.

In the existing Bricolage UI, searching in the job manager is pretty coarse
since you can only search by name. The A-Z selector isn't very useful
because all jobs have names starting with one of "P", "E", or "D" for
Publish, Expire, and Distribute.

We've replaced the Name search and the A-Z selector with searches by Asset
Name, Asset URI, and Scheduler name, which are the things people here were
interested in searching by.

The filter tool is quite nice, but has behaviours which are pretty
inconsistent with the rest of the UI. We kept most of that, with an eye to
reforming the exiting filters later. The change to the filter was to add
an option to see completed jobs, going back a certain number of days.

Please let me know if I need to explain anything better.

thanks,

-mark

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................

Attachments:

smime.p7s (2.17 KB)

david at kineticode

Oct 6, 2009, 2:31 PM

Post #7 of 47 (3533 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Oct 6, 2009, at 5:31 AM, Mark Jaroski wrote:

> Yes, I had it ready to submit as a patch to the list, so I pulled a
> new
> version of the code, branched it, and applied my patch. The patch
> applied
> cleanly so I committed the branch.

Gotcha.

> What:
>
> The Job class now supports searching by attributes of the assets
> associated
> with a publish job. In order to do this cleanly I had to use a LEFT
> JOIN
> with the media_url and story_url tables. This in turn suggested re-
> writing
> the some of the other joins as explicit joins for consistency.

I'd like to see tests for these new features. You can add them to `t/
Bric/Util/Job/Pub/DevTest.pm`. Do all current tests pass with these
changes?

> While I was at it I removed the table aliases to conform with the
> current
> coding standard.

Ah, the table aliases. Gotcha. Woulda been easier to read the diff
without those changes, actually, but I think I can mostly sort them
out in my head.

I'm not sure about joining both story and media at the same time,
though�what's the reason for that? That is, what's the reason for
searching on uri? Also, should the URI comparisons be case-insensitive?

And the search for "username" is weird. Why not use login? The name
format could be all over the map; the use of `lname || ', ' || fname`
is entirely arbitrary.

You removed the appending to $tables under 'grp_id'; doesn't that
break something?

> I'm less confident that you'll accept the UI layer changes, because
> I did
> everything in the mason components rather than trying to get my head
> around
> the callback system. I'll need some coaching to do that.
>
> The changes we made to the UI are below, but a few more words on the
> implementation first: I basically stripped all of the job manager
> code out
> of the generic dhandler, and then created a specific dhandler for
> jobs.
> All of the code implementing the changes in the job UI are isolated
> there.

Okay. It's an okay division of labor, I can see that. The use of the
one big manager dhandler was always a bit hinky.

> Why:
>
> The goal of the UI changes is to make it easier for editors of a
> very large
> Bricolage installation to watch what the other editors are doing,
> mainly
> for quality control. This is particularly necessary in our
> organization
> since we have a very large number of editors from disparate
> departments who
> don't always follow the standards set by the central web-team. The
> web
> team can't handle the additional workload of being part of the
> workflow for
> these units, however they do need to know in general what is getting
> published, so they can spot-check.
>
> In the existing Bricolage UI, searching in the job manager is pretty
> coarse
> since you can only search by name. The A-Z selector isn't very useful
> because all jobs have names starting with one of "P", "E", or "D" for
> Publish, Expire, and Distribute.

Ah, fair point, yes.

> We've replaced the Name search and the A-Z selector with searches by
> Asset
> Name, Asset URI, and Scheduler name, which are the things people
> here were
> interested in searching by.

I see. I think you could use a select list of users and then search by
user id instead of name, though. People will mis-type names all the
time.

> The filter tool is quite nice, but has behaviours which are pretty
> inconsistent with the rest of the UI.

Can you say more about this? I'm a bit confused. At this point, you're
closer to it than I am, so you likely know more about how it works.

> We kept most of that, with an eye to
> reforming the exiting filters later. The change to the filter was
> to add
> an option to see completed jobs, going back a certain number of days.

That sounds useful.

Best,

David

jaroskim at who

Oct 7, 2009, 1:24 AM

Post #8 of 47 (3524 views)
Permalink

Re: Job Manager Improvements [In reply to]

____________________________________
From: David E. Wheeler
Sent: Tue, Oct 06, 2009 at 11:31:34PM +0200
To: To devel [at] lists
Subject: Re: Job Manager Improvements

Hi David,

First off, thanks for your reply. My responses are inline:

> On Oct 6, 2009, at 5:31 AM, Mark Jaroski wrote:
> > The Job class now supports searching by attributes of the assets
> > associated
> > with a publish job. In order to do this cleanly I had to use a LEFT
> > JOIN
> > with the media_url and story_url tables. This in turn suggested re-
> > writing
> > the some of the other joins as explicit joins for consistency.
>
> I'd like to see tests for these new features. You can add them to `t/
> Bric/Util/Job/Pub/DevTest.pm`. Do all current tests pass with these
> changes?

New tests are going to take a while. I know, I should have written them as
I was going, but well. OK, no excuse. Now, however, I've been moved to
another unit, it's a long story but the punchline is that any further
Bricolage work will be on my own time.

> I'm not sure about joining both story and media at the same time,
> though…what's the reason for that? That is, what's the reason for
> searching on uri? Also, should the URI comparisons be case-insensitive?

You can publish either Stories or Media, so if you want to search "by URI"
you have to search both types of URI at the same time.

> You removed the appending to $tables under 'grp_id'; doesn't that
> break something?

I didn't remove it, I just changed it to an explicit join in the FROM
clause to make it more compatible with the LEFT JOIN statements. I think
if it were just me I would probably take it further and make that a view
that does that to simplify the code.

> And the search for "username" is weird. Why not use login? The name
> format could be all over the map; the use of `lname || ', ' || fname`
> is entirely arbitrary.

and later..

> I see. I think you could use a select list of users and then search by
> user id instead of name, though. People will mis-type names all the
> time.

`lname || ', ' || fname` comes from the way the results are displayed. So
the format matches what you see in the Job manager already under the
"Submitter" column. According to the trainer here that will be the easiest
thing to explain to the users. If you do it by login people will try to
guess (and usually get it right), but only after they've typed it the way
they see it once or twice (and gotten mad).

> > The filter tool is quite nice, but has behaviours which are pretty
> > inconsistent with the rest of the UI.
>
> Can you say more about this? I'm a bit confused. At this point, you're
> closer to it than I am, so you likely know more about how it works.

It uses JavaScript to submit the form when you change the filter. This
exact behaviour doesn't really happen anywhere else in the UI that I can
find. There's lots of nice AJAX stuff, but no more auto-submits.

I'm not sure from our User testing reactions that the existing filters are
well-named. I'm not sure there are better names, mind, but many of the
casual users found the names of the filters inconsistent with the
behaviour.

However, I had a limited amount of time to work on this so I ruled out
a complete revision of the filter list. There are lots of neat things that
somebody could do with that though.

thanks!

-mark

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................

Attachments:

smime.p7s (2.17 KB)

david at kineticode

Oct 7, 2009, 10:22 AM

Post #9 of 47 (3513 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Oct 7, 2009, at 1:24 AM, Mark Jaroski wrote:

>> I'd like to see tests for these new features. You can add them to `t/
>> Bric/Util/Job/Pub/DevTest.pm`. Do all current tests pass with these
>> changes?
>
> New tests are going to take a while. I know, I should have written
> them as
> I was going, but well. OK, no excuse. Now, however, I've been
> moved to
> another unit, it's a long story but the punchline is that any further
> Bricolage work will be on my own time.

Okay, I understand. Get to them when you can. I don't feel comfortable
merging in the changes without tests, though.

> You can publish either Stories or Media, so if you want to search
> "by URI"
> you have to search both types of URI at the same time.

Yeah, I realized that. Sad, but assuredly necessary. Fortunately,
they're simple joins to single records, so they should perform quite
well.

> I didn't remove it, I just changed it to an explicit join in the FROM
> clause to make it more compatible with the LEFT JOIN statements. I
> think
> if it were just me I would probably take it further and make that a
> view
> that does that to simplify the code.

I've been thinking about doing that for story, media, and template
views, too, as the queries for those assets, joined to versions and
whatnot, are quite complex -- and I've started using them outside of
the API myself.

However, I thought that the grp_id stuff required that a join be
added, which would mean a table would need to be added to $tables. I
encourage you to run the test suite, though; it will demonstrate
whether I'm right or wrong about that.

> `lname || ', ' || fname` comes from the way the results are
> displayed. So
> the format matches what you see in the Job manager already under the
> "Submitter" column.

Well, no, that format comes from stfname(), which relies on a format
in the preferences. You'd have to replicate that in SQL to make it
work consistently.

> According to the trainer here that will be the easiest
> thing to explain to the users. If you do it by login people will
> try to
> guess (and usually get it right), but only after they've typed it
> the way
> they see it once or twice (and gotten mad).

Not if you give them a select list.

> It uses JavaScript to submit the form when you change the filter.
> This
> exact behaviour doesn't really happen anywhere else in the UI that I
> can
> find. There's lots of nice AJAX stuff, but no more auto-submits.

Well, from the user's POV, whether or not there is a submit is
unimportant. I realize there aren't many auto-submits elsewhere
(though they do exist, for adding and deleting elements, adding
categories, looking up keywords, etc.), but the question is not about
auto-submit. It's about whether the UE is consistent.

> I'm not sure from our User testing reactions that the existing
> filters are
> well-named. I'm not sure there are better names, mind, but many of
> the
> casual users found the names of the filters inconsistent with the
> behaviour.

Would love to hear more on this. Change labels is easy.

> However, I had a limited amount of time to work on this so I ruled out
> a complete revision of the filter list. There are lots of neat
> things that
> somebody could do with that though.

Sure.

Best,

David

jaroskim at who

Oct 8, 2009, 9:29 AM

Post #10 of 47 (3483 views)
Permalink

Re: Job Manager Improvements [In reply to]

____________________________________
From: David E. Wheeler
Sent: Wed, Oct 07, 2009 at 07:22:32PM +0200
To: To devel [at] lists
Subject: Re: Job Manager Improvements

> On Oct 7, 2009, at 1:24 AM, Mark Jaroski wrote:
> Okay, I understand. Get to them when you can. I don't feel comfortable
> merging in the changes without tests, though.

As well you shouldn't. I totally agree.

> > I didn't remove it, I just changed it to an explicit join in the FROM
> > clause to make it more compatible with the LEFT JOIN statements. I
> > think
> > if it were just me I would probably take it further and make that a
> > view
> > that does that to simplify the code.
>
> However, I thought that the grp_id stuff required that a join be
> added, which would mean a table would need to be added to $tables. I
> encourage you to run the test suite, though; it will demonstrate
> whether I'm right or wrong about that.

The table is there. It's always there in fact.

> > `lname || ', ' || fname` comes from the way the results are displayed. So
> > the format matches what you see in the Job manager already under the
> > "Submitter" column.
>
> Well, no, that format comes from stfname(), which relies on a format
> in the preferences. You'd have to replicate that in SQL to make it
> work consistently.

Interesting. That's a new wrinkle for me. I guess it's necessary to add
it to the preference code.

> > According to the trainer here that will be the easiest
> > thing to explain to the users. If you do it by login people will try to
> > guess (and usually get it right), but only after they've typed it the way
> > they see it once or twice (and gotten mad).
>
> Not if you give them a select list.

I'm afraid that a select list would be way too long in our case. It would
probably work fine for smaller user-base though. Maybe it should be a
preference too?

> > I'm not sure from our User testing reactions that the existing filters are
> > well-named. I'm not sure there are better names, mind, but many of the
> > casual users found the names of the filters inconsistent with the
> > behaviour.
>
> Would love to hear more on this. Change labels is easy.

I'll try to get Ed (our trainer) to write something up about it.

thanks again,

-mark

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................

Attachments:

smime.p7s (2.17 KB)

david at kineticode

Oct 9, 2009, 9:34 AM

Post #11 of 47 (3490 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Oct 8, 2009, at 9:29 AM, Mark Jaroski wrote:

>> Not if you give them a select list.
>
> I'm afraid that a select list would be way too long in our case. It
> would
> probably work fine for smaller user-base though. Maybe it should be a
> preference too?

How about a predictive search box like is used when adding keywords?

Best,

David

jaroskim at who

Oct 12, 2009, 12:25 AM

Post #12 of 47 (3446 views)
Permalink

Re: Job Manager Improvements [In reply to]

____________________________________
From: David E. Wheeler
Sent: Fri, Oct 09, 2009 at 06:34:42PM +0200
To: To devel [at] lists
Subject: Re: Job Manager Improvements

> On Oct 8, 2009, at 9:29 AM, Mark Jaroski wrote:
>
> >> Not if you give them a select list.
> >
> > I'm afraid that a select list would be way too long in our case. It
> > would
> > probably work fine for smaller user-base though. Maybe it should be a
> > preference too?
>
> How about a predictive search box like is used when adding keywords?

That would be really nice.

-mark

--
.................................................................
: Mark Jaroski
: Room 9016
: World Health Organization
: +41 22 791 16 65
:
.................................................................

Attachments:

smime.p7s (2.17 KB)

david at kineticode

Oct 12, 2009, 11:30 AM

Post #13 of 47 (3432 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Oct 12, 2009, at 12:25 AM, Mark Jaroski wrote:

>> How about a predictive search box like is used when adding keywords?
>
> That would be really nice.

I eagerly await your updated branch. ;-)

Best,

David

mark at geekhive

Jan 29, 2010, 10:34 AM

Post #14 of 47 (2201 views)
Permalink

Re: Job Manager Improvements [In reply to]

David E. Wheeler wrote:
> I eagerly await your updated branch. ;-)

I've spent a little more time on this since the beginning of the year,
fixing a couple of bugs that were making the existing tests fail. I
committed that code yesterday and pushed it to my fork on Github.

Now I'm going to see about writing some new tests.

One thing about that though: I've taken advantage of a newer feature of
Postgres to rewrite the test tear-down. In the newest Postgres you can
turn off triggers, which effectively means that you can turn off foreign
key constraints. Doing so allows you to just reset the test db with a
simple sql script. :) It might be vaguely slower than deleting specific
ids, but it's much less error prone.

Anyhow, I'm thinking about refactoring at least the jobs tests along the
lines of the book "X-Unit Testing Patterns". I see you've spent a lot of
time on testing over the last few years so I'm interested in your thoughts.

thanks!

-mark

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

david at kineticode

Jan 29, 2010, 11:31 AM

Post #15 of 47 (2213 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Jan 29, 2010, at 10:34 AM, Mark wrote:

> I've spent a little more time on this since the beginning of the year,
> fixing a couple of bugs that were making the existing tests fail. I
> committed that code yesterday and pushed it to my fork on Github.

Okay.

> Now I'm going to see about writing some new tests.

Good!

> One thing about that though: I've taken advantage of a newer feature of
> Postgres to rewrite the test tear-down. In the newest Postgres you can
> turn off triggers, which effectively means that you can turn off foreign
> key constraints. Doing so allows you to just reset the test db with a
> simple sql script. :) It might be vaguely slower than deleting specific
> ids, but it's much less error prone.

Yeah.

> Anyhow, I'm thinking about refactoring at least the jobs tests along the
> lines of the book "X-Unit Testing Patterns". I see you've spent a lot of
> time on testing over the last few years so I'm interested in your thoughts.

Well, I've long thought that we'd be better off if each test was run in a transaction, instead, and there was simply a rollback at the end.

Best,

David

mark at geekhive

Jan 30, 2010, 4:49 AM

Post #16 of 47 (2201 views)
Permalink

Re: Job Manager Improvements [In reply to]

David E. Wheeler wrote:
> > One thing about that though: I've taken advantage of a newer feature of
> > Postgres to rewrite the test tear-down. In the newest Postgres you can
> > turn off triggers, which effectively means that you can turn off foreign
> > key constraints. Doing so allows you to just reset the test db with a
> > simple sql script. :) It might be vaguely slower than deleting specific
> > ids, but it's much less error prone.
>
> Yeah.
>
> > Anyhow, I'm thinking about refactoring at least the jobs tests along the
> > lines of the book "X-Unit Testing Patterns". I see you've spent a lot of
> > time on testing over the last few years so I'm interested in your thoughts.
>
> Well, I've long thought that we'd be better off if each test was run in a
> transaction, instead, and there was simply a rollback at the end.

That's smart. I have some time this afternoon so I'll give it a try.

thanks,

-mark

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

mark at geekhive

Jan 30, 2010, 7:21 AM

Post #17 of 47 (2197 views)
Permalink

Re: Job Manager Improvements [In reply to]

David E. Wheeler wrote:
> > Anyhow, I'm thinking about refactoring at least the jobs tests along the
> > lines of the book "X-Unit Testing Patterns". I see you've spent a lot of
> > time on testing over the last few years so I'm interested in your thoughts.

Another thing I found myself wanting to do was to turn DBI_DEBUG for failed
tests, like so:

ok( $something->do, "Tried to do something: " . debug_do($something));

sub debug_do{
my $something = shift;
# maybe print out some state information here
$something->set_dbi_debug = 1;
$something->do;
$something->set_dbi_debug = 0;
}

Right now that's impossible of course because DBI_DEBUG is a configuration
directive, and a DBI constant. How would you feel about making the above
trick possible? If so what persistent object or singleton do we have to
work with during tests to do this with?

What do you think?

thanks,

-mark

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

mark at geekhive

Jan 30, 2010, 8:04 AM

Post #18 of 47 (2196 views)
Permalink

Re: Job Manager Improvements [In reply to]

David E. Wheeler wrote:
> > Anyhow, I'm thinking about refactoring at least the jobs tests along the
> > lines of the book "X-Unit Testing Patterns". I see you've spent a lot of
> > time on testing over the last few years so I'm interested in your thoughts.
>
> Well, I've long thought that we'd be better off if each test was run in a
> transaction, instead, and there was simply a rollback at the end.

OK, so I see the problem with that now: it's the locking strategy that I
used for executing jobs, which requires explicit commits . Mea culpa.

So I'm trying to come up with a strategy for re-factoring. I don't think we
can get away without having a locking mechanism, and the db still does seem
like a good place for it.

Do you think it would be acceptable to make client code responsible for
doing the commit? It seems a little ugly but it could work like this:

my $whoiam = "$HOSTNAME:$procid"; # where we determine these things earlier

begin;
$job->lock($whoiam); # or maybe $job->claim($whoiam);
commit;

begin;
$job->execute;
commit;

I'm also not keen at all on any scheme which would have a different
behaviour for testing than for normal function, since that at least
partially invalidates the tests.

Or maybe I'm over-stating the need for a locking mechanism at all?

What do you think?

-mark

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

david at kineticode

Jan 30, 2010, 11:03 AM

Post #19 of 47 (2190 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Jan 30, 2010, at 7:21 AM, Mark wrote:

> Another thing I found myself wanting to do was to turn DBI_DEBUG for failed
> tests, like so:
>
> ok( $something->do, "Tried to do something: " . debug_do($something));
>
> sub debug_do{
> my $something = shift;
> # maybe print out some state information here
> $something->set_dbi_debug = 1;
> $something->do;
> $something->set_dbi_debug = 0;
> }
>
> Right now that's impossible of course because DBI_DEBUG is a configuration
> directive, and a DBI constant. How would you feel about making the above
> trick possible? If so what persistent object or singleton do we have to
> work with during tests to do this with?

The test object itself.

> What do you think?

Not sure about the interface, but the concept is sound.

Best,

David

david at kineticode

Jan 30, 2010, 11:05 AM

Post #20 of 47 (2203 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Jan 30, 2010, at 8:04 AM, Mark wrote:

> OK, so I see the problem with that now: it's the locking strategy that I
> used for executing jobs, which requires explicit commits . Mea culpa.
>
> So I'm trying to come up with a strategy for re-factoring. I don't think we
> can get away without having a locking mechanism, and the db still does seem
> like a good place for it.

Agreed. Locking is essential.

> Do you think it would be acceptable to make client code responsible for
> doing the commit? It seems a little ugly but it could work like this:
>
> my $whoiam = "$HOSTNAME:$procid"; # where we determine these things earlier
>
> 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.

> I'm also not keen at all on any scheme which would have a different
> behaviour for testing than for normal function, since that at least
> partially invalidates the tests.

Yeah, agreed.

> Or maybe I'm over-stating the need for a locking mechanism at all?

No, the locks need to stay. But you can circumvent them via tests with Test::MockModule.

Best,

David

mark at geekhive

Jan 30, 2010, 12:17 PM

Post #21 of 47 (2198 views)
Permalink

Re: Job Manager Improvements [In reply to]

David E. Wheeler wrote:
> On Jan 30, 2010, at 8:04 AM, Mark wrote:
>
> > OK, so I see the problem with that now: it's the locking strategy that I
> > used for executing jobs, which requires explicit commits . Mea culpa.
> >
> > So I'm trying to come up with a strategy for re-factoring. I don't think we
> > can get away without having a locking mechanism, and the db still does seem
> > like a good place for it.
>
> Agreed. Locking is essential.
>
> > Do you think it would be acceptable to make client code responsible for
> > doing the commit? It seems a little ugly but it could work like this:
> >
> > my $whoiam = "$HOSTNAME:$procid"; # where we determine these things earlier
> >
> > begin;
> > $job->lock($whoiam); # or maybe $job->claim($whoiam);
> > commit;
> >
> > begin;
> > $job->execute;
> > commit;
>
> No. Bad idea.

OK. I thought so too, but I figured I might as well toss it out there.

> 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.

Very good point. In a way it makes me wonder if it would be possible to
mock the whole DB interaction.

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

david at kineticode

Jan 30, 2010, 3:07 PM

Post #22 of 47 (2200 views)
Permalink

Re: Job Manager Improvements [In reply to]

On Jan 30, 2010, at 12:17 PM, Mark wrote:

> Very good point. In a way it makes me wonder if it would be possible to
> mock the whole DB interaction.

Sure, though that would rather defeat the purpose, since in most cases you want to test the db interaction. But if you don't, you can mock it all away should you wish.

David

jaroskim at who

Feb 4, 2010, 6:08 AM

Post #23 of 47 (2151 views)
Permalink

Re: Job Manager Improvements [In reply to]

____________________________________
From: David E. Wheeler
Sent: Sun, Jan 31, 2010 at 12:07:13AM +0100
To: To devel [at] lists
Subject: Re: Job Manager Improvements

> On Jan 30, 2010, at 12:17 PM, Mark wrote:
>
> > Very good point. In a way it makes me wonder if it would be possible to
> > mock the whole DB interaction.
>
> Sure, though that would rather defeat the purpose, since in most cases
> you want to test the db interaction. But if you don't, you can mock it
> all away should you wish.

Sort of, if the you consider the DB to be included in the system under
test.

However if you want to limit the SUT to just the Bricolage code and
not the db (and schema of course) then mocking all DB interaction would let
you test whether the code produces the correct SQL under a given set of
conditions, without having to actually send that SQL to any db server.

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.

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

jaroskim at who

Feb 4, 2010, 6:13 AM

Post #24 of 47 (2152 views)
Permalink

Re: Job Manager Improvements [In reply to]

____________________________________
From: David E. Wheeler
Sent: Sat, Jan 30, 2010 at 08:05:04PM +0100
To: To devel [at] lists
Subject: Re: Job Manager Improvements

> <...> 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.

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

and here:

http://github.com/MarkJaroski/bricolage/commit/600fdc9f04480083c5abfa0555e60a68f19d88cc

thanks!

-mark

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

david at kineticode

Feb 4, 2010, 9:04 AM

Post #25 of 47 (2145 views)
Permalink

Re: Job Manager Improvements [In reply to]

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

> Sort of, if the you consider the DB to be included in the system under
> test.
>
> However if you want to limit the SUT to just the Bricolage code and
> not the db (and schema of course) then mocking all DB interaction would let
> you test whether the code produces the correct SQL under a given set of
> conditions, without having to actually send that SQL to any db server.

Yes.

> 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.

Best,

David

1 2

View All

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads