Mailing List Archive

On Jun 10, 2011, at 8:27 AM, Mike Raynham wrote:

> If I want to test the result of `find_or_create_alternate()` with MEDIA_UNIQUE_FILENAME enabled and then disabled, how can I do that?

At this point you cannot, except by creating separate branches like you noticed a test was doing with QUEUE_PUBLISH_JOBS. I regret implementing configuration data as constants nowadays.

> I could write a test that checks the config setting, and runs the appropriate test. But to run both tests would require manually changing the configuration setting, and re-running the test.

Right.

> I've experimented with redefining constants, but that didn't work out well - they are constants after all.

Right.

> I have also experimented with using AUTOLOAD to create a getter method for the config variables instead (see attached MyApp example). This allows me to temporarily redefine the sub on-the-fly so that it returns the value I want. This works, but would ultimately require all the config constants to be replaced with calls to their getter method.

Well, the one you're interested in at least.

> Is there a good way to write this type of test?

Alas, no, not as long as configuration directives are represented as constants.

Best,

David

On 10 Jun 2011 23:01, "David E. Wheeler" <david@kineticode.com> wrote:
>
> On Jun 10, 2011, at 8:27 AM, Mike Raynham wrote:
>
> > If I want to test the result of `find_or_create_alternate()` with
MEDIA_UNIQUE_FILENAME enabled and then disabled, how can I do that?
>
> At this point you cannot, except by creating separate branches like you
noticed a test was doing with QUEUE_PUBLISH_JOBS. I regret implementing
configuration data as constants nowadays.
>
> > I could write a test that checks the config setting, and runs the
appropriate test. But to run both tests would require manually changing the
configuration setting, and re-running the test.
>
> Right.
>
> > I've experimented with redefining constants, but that didn't work out
well - they are constants after all.
>
> Right.
>
> > I have also experimented with using AUTOLOAD to create a getter method
for the config variables instead (see attached MyApp example). This allows
me to temporarily redefine the sub on-the-fly so that it returns the value I
want. This works, but would ultimately require all the config constants to
be replaced with calls to their getter method.
>
> Well, the one you're interested in at least.
>
> > Is there a good way to write this type of test?
>
> Alas, no, not as long as configuration directives are represented as
constants.
>

Would you be happy for me to submit a patch for Bric::Config so that
configuration directives can be obtained via an AUTOLOAD getter method?

Configuration directives can then be migrated as and when required.

Regards,

Mike

On Jun 10, 2011, at 8:40 PM, Mike Raynham wrote:

> Would you be happy for me to submit a patch for Bric::Config so that
> configuration directives can be obtained via an AUTOLOAD getter method?
>
> Configuration directives can then be migrated as and when required.

No autoload, please.

We could maybe set something up, though, so that they get created as functions rather than constants. Then you could replace them at runtime using Test::MockModule.

Best,

David

On 11/06/11 18:38, David E. Wheeler wrote:
> On Jun 10, 2011, at 8:40 PM, Mike Raynham wrote:
>
>> Would you be happy for me to submit a patch for Bric::Config so that
>> configuration directives can be obtained via an AUTOLOAD getter method?
>>
>> Configuration directives can then be migrated as and when required.
>
> No autoload, please.
>
> We could maybe set something up, though, so that they get created as functions rather than constants. Then you could replace them at runtime using Test::MockModule.

My plan is to create an AUTOLOAD function in Bric::Config which will
replace each of the constants with a getter function. I've attached a
diff of the Bric::Config code that I've been experimenting with so far.


if (MEDIA_UNIQUE_FILENAME) { ... }

would be replaced with:

if (Bric::Config::get_media_unique_filename) { ... }


The functions could be exported, but I thought it might be clearer (if a
little longer) to use the fully qualified name.

As there are lots of constants throughout the code, they can be switched
out as and when required, rather than all at once. The AUTOLOAD
function throws an exception if the configuration directive is defined
as a constant, so it shouldn't be possible to use both at the same time.

Many of the constants can simply be removed or commented out, and they
will automatically be available via a getter function. In some cases,
default values are set at the point at which the constant is created.
In these cases, the $config hash value will need setting or updating as
appropriate. For example:

use constant MEDIA_URI_ROOT => '/data/media';

would have to be changed to:

$config->{MEDIA_URI_ROOT} = '/data/media';

The functions created by AUTOLOAD can then be overridden with
Test::MockModule.

Is this what you were thinking?

I've started a DevTest module for Bric::Config too. At the moment it
just checks that AUTOLOAD throws the correct exceptions. I'm sure it
could be enhanced later.



Regards,

Mike

On Jun 11, 2011, at 11:34 AM, Mike Raynham wrote:

> My plan is to create an AUTOLOAD function in Bric::Config which will replace each of the constants with a getter function. I've attached a diff of the Bric::Config code that I've been experimenting with so far.
>
>
> if (MEDIA_UNIQUE_FILENAME) { ... }
>
> would be replaced with:
>
> if (Bric::Config::get_media_unique_filename) { ... }
>
>
> The functions could be exported, but I thought it might be clearer (if a little longer) to use the fully qualified name.

I think you're creating a *lot* of work for yourself that way. Better I think would be to replace all the `use constant` lines in Bric::Config with `sub` lines. Then the interface would be the same, but they could be overridden at runtime.

> As there are lots of constants throughout the code, they can be switched out as and when required, rather than all at once.

Yes, that would be substantially less work, of course. Or substantially less up-front, I should say.

> The AUTOLOAD function throws an exception if the configuration directive is defined as a constant, so it shouldn't be possible to use both at the same time.

Yeah. I still wouldn't use AUTOLOAD, though. Not literally, that is.

> Many of the constants can simply be removed or commented out, and they will automatically be available via a getter function. In some cases, default values are set at the point at which the constant is created. In these cases, the $config hash value will need setting or updating as appropriate. For example:
>
> use constant MEDIA_URI_ROOT => '/data/media';
>
> would have to be changed to:
>
> $config->{MEDIA_URI_ROOT} = '/data/media';
>
> The functions created by AUTOLOAD can then be overridden with Test::MockModule.
>
> Is this what you were thinking?

No. I would change it to

eval "sub MEDIA_URI_ROOT { '/data/media' }";

Then you get the same result with *not* change to the interface.

> I've started a DevTest module for Bric::Config too. At the moment it just checks that AUTOLOAD throws the correct exceptions. I'm sure it could be enhanced later.

(More tests)++. Hell, if you're writing tests, you don't have to do it my way, because your way is more likely to work forever. :-)

Best,

David

On 11/06/11 21:24, David E. Wheeler wrote:
> On Jun 11, 2011, at 11:34 AM, Mike Raynham wrote:
>
>> My plan is to create an AUTOLOAD function in Bric::Config which will replace each of the constants with a getter function. I've attached a diff of the Bric::Config code that I've been experimenting with so far.
>>
>>
>> if (MEDIA_UNIQUE_FILENAME) { ... }
>>
>> would be replaced with:
>>
>> if (Bric::Config::get_media_unique_filename) { ... }
>>
>>
>> The functions could be exported, but I thought it might be clearer (if a little longer) to use the fully qualified name.
>
> I think you're creating a *lot* of work for yourself that way. Better I think would be to replace all the `use constant` lines in Bric::Config with `sub` lines. Then the interface would be the same, but they could be overridden at runtime.

I agree, and simply replacing constants with subs was my first thought.
But then I started to think about Perl naming conventions, and it
began to feel a bit wrong - even though the subs are essentially doing
the same thing as the constants.

>
>> As there are lots of constants throughout the code, they can be switched out as and when required, rather than all at once.
>
> Yes, that would be substantially less work, of course. Or substantially less up-front, I should say.

I was planning on leaving the bulk of the work to others... ;-)

>
>> The AUTOLOAD function throws an exception if the configuration directive is defined as a constant, so it shouldn't be possible to use both at the same time.
>
> Yeah. I still wouldn't use AUTOLOAD, though. Not literally, that is.
>
>> Many of the constants can simply be removed or commented out, and they will automatically be available via a getter function. In some cases, default values are set at the point at which the constant is created. In these cases, the $config hash value will need setting or updating as appropriate. For example:
>>
>> use constant MEDIA_URI_ROOT => '/data/media';
>>
>> would have to be changed to:
>>
>> $config->{MEDIA_URI_ROOT} = '/data/media';
>>
>> The functions created by AUTOLOAD can then be overridden with Test::MockModule.
>>
>> Is this what you were thinking?
>
> No. I would change it to
>
> eval "sub MEDIA_URI_ROOT { '/data/media' }";
>
> Then you get the same result with *not* change to the interface.
>
>> I've started a DevTest module for Bric::Config too. At the moment it just checks that AUTOLOAD throws the correct exceptions. I'm sure it could be enhanced later.
>
> (More tests)++. Hell, if you're writing tests, you don't have to do it my way, because your way is more likely to work forever. :-)

I'm torn on this one. I think in the long run, AUTOLOAD with getter
functions that follow good naming conventions is a clean way to do it.

On the other hand, I agree that simply swapping out 'use constant' for
subs is a helluva lot simpler. It also means that the code won't be in
a state of limbo, with constants in some places, and getter functions in
others.

What do you mean when you say that my way is more likely to work forever?


Regards,

Mike

On Jun 11, 2011, at 2:15 PM, Mike Raynham wrote:

> I'm torn on this one. I think in the long run, AUTOLOAD with getter functions that follow good naming conventions is a clean way to do it.

I disagree. AUTOLOAD should be avoided.

> On the other hand, I agree that simply swapping out 'use constant' for subs is a helluva lot simpler. It also means that the code won't be in a state of limbo, with constants in some places, and getter functions in others.

Right. And you can start using it fully-qualified, too, if you want.

> What do you mean when you say that my way is more likely to work forever?

I mean if there are tests, it's less likely to break in the future. At the moment, IIRC, there are no tests for Bric::Config.

Best,

David

On 12/06/11 03:56, David Wheeler wrote:
> On Jun 11, 2011, at 2:15 PM, Mike Raynham wrote:
>
>> I'm torn on this one. I think in the long run, AUTOLOAD with getter functions that follow good naming conventions is a clean way to do it.
>
> I disagree. AUTOLOAD should be avoided.
>
>> On the other hand, I agree that simply swapping out 'use constant' for subs is a helluva lot simpler. It also means that the code won't be in a state of limbo, with constants in some places, and getter functions in others.
>
> Right. And you can start using it fully-qualified, too, if you want.
>
>> What do you mean when you say that my way is more likely to work forever?
>
> I mean if there are tests, it's less likely to break in the future. At the moment, IIRC, there are no tests for Bric::Config.

Having slept on it, I'm in agreement with you. I'll just swap out the
constants for subs, and that should do it. Changing the interface is
bound to lead to problems.


I'll try and write some useful tests for Bric::Config too.

Thanks for your help.


Regards,

Mike

On Jun 12, 2011, at 12:44 AM, Mike Raynham wrote:

> Having slept on it, I'm in agreement with you. I'll just swap out the constants for subs, and that should do it. Changing the interface is bound to lead to problems.
>
> I'll try and write some useful tests for Bric::Config too.

Mike++

Best,

David