Bug #116909 for DateTime-TimeZone-Local-Win32: Tests easy to fall out of sync with DateTime::TimeZone

Thu Aug 11 15:23:57 2016 mschwern [...] cpan.org - Ticket created

Subject:

Tests easy to fall out of sync with DateTime::TimeZone

t\01-local-win32.t is currently failing on Strawberry Perl 5.20.1 on Windows 7 Ultimate.

Four time zones cannot be found by t\01-local-win32.t
* Altai Standard Time
* Astrakhan Standard Time
* Bougainville Standard Time
* Tomsk Standard Time

This is because I have DateTime::TimeZone 1.74 installed which doesn't know about these time zones. 2.01 has all these time zones (for example, Altai is DateTime::TimeZone::Asia::Barnaul).

t\01-local-win32.t seems to test that DateTime::TimeZone knows about every time zone in the Windows registry. This would seem to guarantee test failure.

I'd recommend:
* Only test at install time that the common time zones work.
* Move the full check to an author test, use it to keep DateTime::TimeZone up to date.

Thu Aug 11 15:43:09 2016 mschwern [...] cpan.org - Correspondence added

PS You can see this failure in https://ci.appveyor.com/project/HayoBaan/dist-zilla-pluginbundle-author-hayobaan/build/1.0.1

For some reason Dist::Zilla (which depends on DateTime) is pulling in DateTime::TimeZone::Local::Win32 but not updating DateTime::TimeZone.

Thu Aug 11 16:14:28 2016 ether [...] cpan.org - Correspondence added

On 2016-08-11 12:43:09, MSCHWERN wrote: Show quoted text

> PS You can see this failure in > https://ci.appveyor.com/project/HayoBaan/dist-zilla-pluginbundle- > author-hayobaan/build/1.0.1 > > For some reason Dist::Zilla (which depends on DateTime) is pulling in > DateTime::TimeZone::Local::Win32 but not updating DateTime::TimeZone.

Dist::Zilla has a Win32-specific prereq on DateTime::TimeZone::Local::Win32, but does not require any specific version of DateTime::TimeZone.

Thu Aug 11 16:14:28 2016 The RT System itself - Status changed from 'new' to 'open'

Thu Aug 11 20:57:34 2016 mschwern [...] cpan.org - Correspondence added

On Thu Aug 11 16:14:28 2016, ETHER wrote:
Show quoted text

> Dist::Zilla has a Win32-specific prereq on
> DateTime::TimeZone::Local::Win32, but does not require any specific
> version of DateTime::TimeZone.

Now that DateTime::TimeZone depends on DateTime::TimeZone::Local::Win32, should anything depend directly on DateTime::TimeZone::Local::Win32?

Thu Aug 11 21:34:38 2016 mschwern [...] cpan.org - Correspondence added

See https://github.com/rjbs/Dist-Zilla/pull/563

Thu Aug 11 21:50:36 2016 mschwern [...] cpan.org - Correspondence added

Turns out it's not Dist::Zilla's fault.

Here's `cpanm DateTime::TimeZone` failing on a fresh Strawberry Perl.
https://ci.appveyor.com/project/schwern/dist-zilla/build/1.0.6

It's trying to install DateTime::TimeZone.

0. An older version of DateTime::TimeZone is already installed (ships with Strawberry Perl).
1. cpanm sees DateTime::TimeZone::Local::Win32 as a dependency.
2. cpanm tests DateTime::TimeZone::Local::Win32.
3. DateTime::TimeZone::Local::Win32 sees DateTime::TimeZone::Local is installed and runs its tests.
4. The tests fail because the installed DateTime::TimeZone is out of date.

That sort of circular dependency means DateTime::TimeZone::Local::Win32 can't usefully test itself until after DateTime::TimeZone is installed.

I'd recommend that...
1) All but basic tests be moved into xt/
2) The tests check for a minimum version of DateTime::TimeZone::Local
3) Consider putting DateTime::TimeZone::Local::Win32 back into the DateTime::TimeZone dist.

Thu Aug 11 21:53:05 2016 DROLSKY [...] cpan.org - Correspondence added

On Thu Aug 11 21:50:36 2016, MSCHWERN wrote: Show quoted text

> Turns out it's not Dist::Zilla's fault. > > Here's `cpanm DateTime::TimeZone` failing on a fresh Strawberry Perl. > https://ci.appveyor.com/project/schwern/dist-zilla/build/1.0.6 > > It's trying to install DateTime::TimeZone. > > 0. An older version of DateTime::TimeZone is already installed (ships > with > Strawberry Perl). > 1. cpanm sees DateTime::TimeZone::Local::Win32 as a dependency. > 2. cpanm tests DateTime::TimeZone::Local::Win32. > 3. DateTime::TimeZone::Local::Win32 sees DateTime::TimeZone::Local is > installed > and runs its tests. > 4. The tests fail because the installed DateTime::TimeZone is out of > date. > > That sort of circular dependency means > DateTime::TimeZone::Local::Win32 can't > usefully test itself until after DateTime::TimeZone is installed. > > I'd recommend that... > 1) All but basic tests be moved into xt/ > 2) The tests check for a minimum version of DateTime::TimeZone::Local > 3) Consider putting DateTime::TimeZone::Local::Win32 back into the > DateTime::TimeZone dist.

#3 is not an option. We moved it out specifically so someone who has Win32 knowledge can maintain it and release it on its own schedule, without depending on me. #1 and #2 sound reasonable to me.

Thu Aug 11 22:23:43 2016 mschwern [...] cpan.org - Correspondence added

1 and 2 done in https://github.com/dapink/DateTime-TimeZone-Local-Win32/pull/4

Thu Aug 11 23:13:04 2016 ether [...] cpan.org - Correspondence added

On 2016-08-11 17:57:34, MSCHWERN wrote: Show quoted text

> On Thu Aug 11 16:14:28 2016, ETHER wrote:

> > Dist::Zilla has a Win32-specific prereq on > > DateTime::TimeZone::Local::Win32, but does not require any specific > > version of DateTime::TimeZone.

> > Now that DateTime::TimeZone depends on > DateTime::TimeZone::Local::Win32, should > anything depend directly on DateTime::TimeZone::Local::Win32?

The dependency was added to fix https://github.com/rjbs/Dist-Zilla/issues/497.

Fri Aug 12 01:37:35 2016 DAPINK [...] cpan.org - Correspondence added

On Thu Aug 11 22:23:43 2016, MSCHWERN wrote: Show quoted text

> 1 and 2 done in https://github.com/dapink/DateTime-TimeZone-Local- > Win32/pull/4

I've replicated the problem and know where to fix it, but I will not be accepting the code change. The test did check for a minimum DT-TZ version, but I simply made a mistake and used it where it was not intended. I do not want to make the tests author tests for an important reason. At some point, Microsoft will introduce new time zones and if I fail to handle that case first, I want a test failure to pick it up. I've reserved author tests for those cases where I depend on the time zones in the test OS to be up to date. As far as the comment regarding the lexical variable possibly not working as intended, the use of "local" is to localize one of the values. See #3 under http://perldoc.perl.org/perlsub.html#When-to-Still-Use-local() I will plan to get a fix out in the next day or so after I verify whether other changes are needed.

Fri Aug 12 01:37:40 2016 DAPINK [...] cpan.org - Taken

Fri Aug 12 15:43:46 2016 DAPINK [...] cpan.org - Severity Important added

Fri Aug 12 15:43:46 2016 DAPINK [...] cpan.org - Broken in 1.91 added

Fri Aug 12 17:29:28 2016 DAPINK [...] cpan.org - Status changed from 'open' to 'resolved'

Fri Aug 12 17:29:34 2016 DAPINK [...] cpan.org - Fixed in 1.92 added

Fri Aug 12 19:18:44 2016 ether [...] cpan.org - Correspondence added

On 2016-08-11 22:37:35, DAPINK wrote: Show quoted text

> I do not want to make the tests author tests for an important reason. > At some point, Microsoft will introduce new time zones and if I fail > to handle that case first, I want a test failure to pick it up. I've > reserved author tests for those cases where I depend on the time zones > in the test OS to be up to date. >

You could make the tests run on smokers (that is, the machines running in the cpantesters infrastructure, that give us the PASS and FAIL reports), but still not block user installations, by checking for $ENV{AUTOMATED_TESTING}.

Fri Aug 12 19:34:18 2016 DAPINK [...] cpan.org - Correspondence added

On Fri Aug 12 19:18:44 2016, ETHER wrote: Show quoted text

> You could make the tests run on smokers (that is, the machines running > in the cpantesters infrastructure, that give us the PASS and FAIL > reports), but still not block user installations, by checking for > $ENV{AUTOMATED_TESTING}.

That makes a lot of sense. I'll create a new release with that change. Thanks.