t/steps/*/*.t: Replace reliance on init::defaults

[jkeenan]

This ticket moves to Trac discussion of issues found in several tickets in the older RT system, principally:

*  t/configure/1*.t tests incorrectly rely on init::defaults, opened by Andy Dougherty in November 2007.

*  Remove config::init::defaults From configure tests, opened by chromatic, also in November 2007.

Discussion in these RTs was, to say the least, extensive. Let me quote from the original posts in the two tickets to give a flavor of the issues. The tests in question have moved to a different directory structure since these RTs were originally filed. I've reflected those file movements in the following citations.

Andy Dougherty:

[The tests in t/steps/] rely on init::defaults. That simply won't work if the configuration being used to build parrot is sufficiently different from that which was used to build perl. That's what happened to me in this case.

This probably doesn't show up often because nearly everyone building parrot right now uses the same configuration as was used to build perl. However, that's not necessarily the norm. Vendors such as Sun supply a perl pre-built with their compiler, but many end users have gcc installed instead.

If a test for a configuration step relies on certain Config values, the only correct approach is to use the Config values determined by Configure.pl. There is no guarantee that the init::defaults values will work. They may well have been changed, either by the user (via --ask or command line overrides) or by the Configure.pl system itself.

chromatic:

config::init::defaults pulls the libs setting out of the Perl 5 configuration and various tests of the Parrot configuration process use config::init::defaults to set up the environment for testing.

When these tests attempt to compile and link programs, they may fail because the Perl 5 configuration may not reflect the actual run-time environment of the code.

Parrot::Config::Generated is a much more reliable source of information, and if the tests truly need information about the local system for compiling and linking purposes, they should fetch the information from there, not from the Perl 5 configuration which does not necessarily reflect the state of the local machine.

I can imagine an objection to this suggestion, specifically But these tests should be runnable without having previously configured Parrot! We cannot rely on the configuration process working correctly unless we can test that process'' [Comment: This has indeed been my basic objection -- kid51] Yet if we cannot rely on the accuracy of the tests, they provide little value. Clearly Parrot's configuration process works on my machine, as demonstrated by the fact that Parrot builds successfully and passes all functional tests. Yet the configuration test fails with a false negative.

To get the maximum value out of the tests of the configuration system, the tests must be robust and reliable. Failures should indicate actual problems of behavior. I don't believe that we can rely on these tests until we remove the assumption that the local Perl 5 configuration is correct for Parrot.

Thank you very much.

kid51

[jkeenan]

Created  https://svn.parrot.org/parrot/branches/configtests to work on this ticket.

[doughera]

Yes, I think you have accurately summarized the issues. I regret that I will likely be unavailable to work on this topic in the foreseeable future. Parrot no longer builds on that system, and I don't anticipate having any time to devote to fixing and maintaining it. You can mimic the problem using the Config.pm trickery I describe in the original ticket  http://rt.perl.org/rt3/Ticket/Display.html?id=47391.

[jkeenan]

Replying to jkeenan:

Created  https://svn.parrot.org/parrot/branches/configtests to work on this ticket.

I plan to merge the configtests branch into trunk right after the 20091117 release.

This branch accomplishes the following:

* Calling perl Configure.pl --test (or with test=configure) will now run the tests in t/steps/*.t after Configure.pl itself has completed.

* Instead of running configuration step init::defaults (and others, as needed) to provide reasonable defaults to a test file's Parrot::Configure object, each test file will consult the results of Parrot's configuration: %Parrot::Config::Pconfig. In many step test files, this will have the effect of reducing the total number of tests run by five per config step eliminated.

* Calls to the Perl 5 %Config in configuration step classes falling later than init::defaults have been eliminated, as have calls to %Config in the corresponding step tests. This includes both direct calls (e.g., use Config;) as well as indirect calls using Parrot::Configure::Data::get_p5(). Setting aside a few usages of Parrot::Configure::Data::keys_p5(), we have now quarantined calls to the Perl 5 %Config to init::defaults and its corresponding test file.

* That quarantining, however, came at a price. Up until now, in cases where we knew we needed values from %Config during later configuration steps or step tests, we stored them in a special part of the Parrot::Configure object -- special in the sense that its contents were not transferred to %Parrot::Config::Config in the final configuration step. We want to have these values available as needed -- but we also don't want them used during make or make test because there ultimate source is something we ultimately want to replace. So these values will now be added to the main part of the Parrot::Configure object and will therefore be transferred into %Parrot::Config::Config -- but they will have _provisional appended to their names. The _provisional designation will be a signal: Don't rely on this for building Parrot or testing Parrot; it's only a (medium- to long-term) crutch used in configuring Parrot.

* In the course of working on this ticket, I created and resolved other tickets which (a) removed some attribute-determining logic from gen::platform and placed it in auto::arch (TT #1263); (b) eliminated calls to %Config in test files run under make test (TT #1234 and TT #1235); cleared up other locations with an improper dependence on %Config (TT #1194 and TT #1236).

I will be attaching a gzipped file of a recent diff between this branch and trunk at the branching point.

Thank you very much.

kid51

[jkeenan]

gzipped diff between trunk and configtests branch

[jkeenan]

Replying to jkeenan:

Replying to jkeenan: So these values will now be added to the main part of the Parrot::Configure object and will therefore be transferred into %Parrot::Config::Config -- but they will have _provisional appended to their names. The _provisional designation will be a signal: Don't rely on this for building Parrot or testing Parrot; it's only a (medium- to long-term) crutch used in configuring Parrot.

The _provisional tag was overapplied. Getting build failures. Will diagnose, correct and post corrected diff.

kid51

[jkeenan]

Replying to jkeenan:

The _provisional tag was overapplied. Getting build failures. Will diagnose, correct and post corrected diff.

A corrected patch was merged into trunk at r42575.

Now recommended: make realclean && svn up && perl Configure.pl --test

I will keep this TT open for a little while to:

* Get bug reports * Improve test coverage where possible * Touch-ups to docs

Thank you very much.

kid51

[jkeenan]

No complaints received. In r42601, made explicit that new configuration steps should be proposed via Trac tickets. Touched up docs. Closing ticket.