Closed p5pRT closed 12 years ago
The patch attached is the last of those begun at yesterday's St Louis Perl Hackathon. Thanks to all participants in that event.
The patch converts the file from using all hard-coded tests (or subroutines wrapping around hard-coded tests) to using functions pulled in by require-ing test.pl.
140 of the 167 individual tests lacked descriptions. To a certain extent\, I composed descriptions from inline comments\, then removed those comments. I tried to make the test descriptions as unique as possible so that someone debugging a test failure could more easily locate the failure.
Since we presume that the tests were accurate to begin with\, in reviewing please first look for cases where the description is truly wrong\, and then look for cases where the description is capable of improvement.
I would like to be able to merge this into blead in 7 days. Thank you very much.
Jim Keenan
On Sun\, Nov 18\, 2012 at 11:49 AM\, James E Keenan \perlbug\-followup@​perl\.org wrote:
# New Ticket Created by James E Keenan # Please include the string: [perl #115806] # in the subject line of all future correspondence about this issue. # \<URL: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=115806 >
The patch attached is the last of those begun at yesterday's St Louis Perl Hackathon. Thanks to all participants in that event.
The patch converts the file from using all hard-coded tests (or subroutines wrapping around hard-coded tests) to using functions pulled in by require-ing test.pl.
140 of the 167 individual tests lacked descriptions. To a certain extent\, I composed descriptions from inline comments\, then removed those comments. I tried to make the test descriptions as unique as possible so that someone debugging a test failure could more easily locate the failure.
Since we presume that the tests were accurate to begin with\, in reviewing please first look for cases where the description is truly wrong\, and then look for cases where the description is capable of improvement.
I would like to be able to merge this into blead in 7 days. Thank you very much.
Jim Keenan
If you (temporarily) increment $Level in sloppy_eq() then the line numbers in "not ok" messages will be more helpful.
sub sloppy_eq { my ($got\, $expected\, $message) = @_; my $result = 0; if ($got == $expected) { $result = 1; } else { my $error = abs (($got - $expected) / $got); if ($error \< 1e-9) { $result = 1; } } + local $Level = $Level + 1; ok $result\, $message; }
The RT System itself - Status changed from 'new' to 'open'
On Sun Nov 18 09:49:09 2012\, jkeen@verizon.net wrote:
The patch attached is the last of those begun at yesterday's St Louis Perl Hackathon. Thanks to all participants in that event.
The patch converts the file from using all hard-coded tests (or subroutines wrapping around hard-coded tests) to using functions pulled in by require-ing test.pl.
But test.pl uses infix + for its machinery\, and we are trying to test that here.
--
Father Chrysostomos
On Sun Nov 18 11:02:39 2012\, sprout wrote:
On Sun Nov 18 09:49:09 2012\, jkeen@verizon.net wrote:
The patch attached is the last of those begun at yesterday's St Louis Perl Hackathon. Thanks to all participants in that event.
The patch converts the file from using all hard-coded tests (or subroutines wrapping around hard-coded tests) to using functions pulled in by require-ing test.pl.
But test.pl uses infix + for its machinery\, and we are trying to test that here.
We were following the guidance provided by this passage in pod/perlhack.pod:
##### =item * F\<t/cmd>\, F\<t/run>\, F\<t/io> and F\<t/op> Now that basic require() and subroutines are tested\, you can use the F\<t/test.pl> library.
You can also use certain libraries like Config conditionally\, but be sure to skip the test gracefully if it's not there. #####
I infer from what you say that this guidance is wrong. If so\, can you please provide a patch ASAP that correctly describes when we can use 't/test.pl'? Otherwise\, people whom we are trying to introduce to the Perl 5 core distribution will have their patches rejected and be discouraged from making further contributions.
Thank you very much. Jim Keenan
On Sun Nov 18 18:34:04 2012\, jkeenan wrote:
On Sun Nov 18 11:02:39 2012\, sprout wrote:
On Sun Nov 18 09:49:09 2012\, jkeen@verizon.net wrote:
The patch attached is the last of those begun at yesterday's St Louis Perl Hackathon. Thanks to all participants in that event.
The patch converts the file from using all hard-coded tests (or subroutines wrapping around hard-coded tests) to using functions pulled in by require-ing test.pl.
But test.pl uses infix + for its machinery\, and we are trying to test that here.
We were following the guidance provided by this passage in pod/perlhack.pod:
##### =item * F\<t/cmd>\, F\<t/run>\, F\<t/io> and F\<t/op> Now that basic require() and subroutines are tested\, you can use the F\<t/test.pl> library.
You can also use certain libraries like Config conditionally\, but be sure to skip the test gracefully if it's not there. #####
I infer from what you say that this guidance is wrong. If so\, can you please provide a patch ASAP that correctly describes when we can use 't/test.pl'? Otherwise\, people whom we are trying to introduce to the Perl 5 core distribution will have their patches rejected and be discouraged from making further contributions.
I have patched it in commit b1b0854bd000.
--
Father Chrysostomos
On Sun Nov 18 20:20:29 2012\, sprout wrote:
On Sun Nov 18 18:34:04 2012\, jkeenan wrote:
On Sun Nov 18 11:02:39 2012\, sprout wrote:
On Sun Nov 18 09:49:09 2012\, jkeen@verizon.net wrote:
The patch attached is the last of those begun at yesterday's St Louis Perl Hackathon. Thanks to all participants in that event.
The patch converts the file from using all hard-coded tests (or subroutines wrapping around hard-coded tests) to using functions pulled in by require-ing test.pl.
But test.pl uses infix + for its machinery\, and we are trying to test that here.
We were following the guidance provided by this passage in pod/perlhack.pod:
##### =item * F\<t/cmd>\, F\<t/run>\, F\<t/io> and F\<t/op> Now that basic require() and subroutines are tested\, you can use the F\<t/test.pl> library.
You can also use certain libraries like Config conditionally\, but be sure to skip the test gracefully if it's not there. #####
I infer from what you say that this guidance is wrong. If so\, can you please provide a patch ASAP that correctly describes when we can use 't/test.pl'? Otherwise\, people whom we are trying to introduce to the Perl 5 core distribution will have their patches rejected and be discouraged from making further contributions.
I have patched it in commit b1b0854bd000.
Father C:
While I thank you for your quick response\, I don't think the patch clarifies matters sufficiently:
This means that the would-be test writer must first conduct a study of t/test.pl and list all the Perl features it uses\, then inspect the particular test file to see whether it avoids those features.
When I opened up t/op/arith.t this weekend\, there were no comments or POD in it to indicate that it could not be tested with t/test.pl. And its placement in t/op/ indicated that it could be so tested.
If there are files such as t/op/arith.t that currently sit in a directory listed as testable by t/test.pl which are not\, in fact\, so testable\, then let's move them into another directory or to one already listed as requiring hard-coded tests only. That way\, the would-be test description writer doesn't have to get bogged down in an analysis of the theoretical limits of t/test.pl.
Thank you very much. Jim Keenan
On Mon Nov 19 05:08:26 2012\, jkeenan wrote:
Father C:
While I thank you for your quick response\, I don't think the patch clarifies matters sufficiently:
diff --git a/pod/perlhack.pod b/pod/perlhack.pod index 64329ed..9f2513f 100644 --- a/pod/perlhack.pod +++ b/pod/perlhack.pod @@ -713\,6 +713\,9 @@ tested. Now that basic require() and subroutines are tested\, you can use the F\<t/test.pl> library.
+Note\, however\, that some test scripts still avoid F\<t/test.pl> if they make +use of features that F\<t/test.pl> relies on heavily. +
This means that the would-be test writer must first conduct a study of t/test.pl and list all the Perl features it uses\, then inspect the particular test file to see whether it avoids those features.
When I opened up t/op/arith.t this weekend\, there were no comments or POD in it to indicate that it could not be tested with t/test.pl. And its placement in t/op/ indicated that it could be so tested.
If there are files such as t/op/arith.t that currently sit in a directory listed as testable by t/test.pl which are not\, in fact\, so testable\, then let's move them into another directory or to one already listed as requiring hard-coded tests only.
I would not be opposed to that. I hope you are not expected me to do it\, though. :-)
I do think you are reading too much into what perlhack says. test.pl may be OK to use in op/\, but that does not mean that everything in there must use it.
Still\, separation into different directories does not sound like a bad idea.
That way\, the would-be test description writer doesn't have to get bogged down in an analysis of the theoretical limits of t/test.pl.
--
Father Chrysostomos
On Mon Nov 19 08:47:17 2012\, sprout wrote:
I do think you are reading too much into what perlhack says. test.pl may be OK to use in op/\, but that does not mean that everything in there must use it.
Still\, separation into different directories does not sound like a bad idea.
I have created https://rt-archive.perl.org/perl5/Ticket/Display.html?id=115838 to address the general issue. Have you seen that yet?
Thank you very much. Jim Keenan
On Sun Nov 18 11:02:39 2012\, sprout wrote:
On Sun Nov 18 09:49:09 2012\, jkeen@verizon.net wrote:
The patch attached is the last of those begun at yesterday's St Louis Perl Hackathon. Thanks to all participants in that event.
The patch converts the file from using all hard-coded tests (or subroutines wrapping around hard-coded tests) to using functions pulled in by require-ing test.pl.
But test.pl uses infix + for its machinery\, and we are trying to test that here.
I am attaching a patch which *substitutes* for the patch previously attached to this RT. Father C\, I believe it should satisfy your concerns.
Please review (anyone\, not just Father C)\, as we had to write descriptions for 140 out of 167 individual tests in this file.
Thank you very much. Jim Keenan
On Fri Nov 23 13:24:27 2012\, jkeenan wrote:
On Mon Nov 19 08:47:17 2012\, sprout wrote:
I do think you are reading too much into what perlhack says. test.pl may be OK to use in op/\, but that does not mean that everything in there must use it.
Still\, separation into different directories does not sound like a bad idea.
I have created https://rt-archive.perl.org/perl5/Ticket/Display.html?id=115838 to address the general issue. Have you seen that yet?
I did. I have no comment on that in particular. Let me explain why.
I have seen test suite refactorings that basically sabotaged the tests\, so they were no longer testing what they were supposed to be testing. That actually happened in 5.10. A feature was added\, with tests. The tests were refactored. Later the feature was broken and 5.10 shipped with a broken feature\, but the tests still passed. So Iām paranoid.
I donāt fully understand or appreciate the reasons why certain things are done certain ways\, but I can appreciate that there may be reasons. So I donāt assume that all changes are safe to make.
I have seen comments scattered throughout the test suite suggesting that things are done oddly for a reason\, so I try to be careful when it comes to modifying tests.
I just wanted to share my paranoia with others by raising awareness of the issues. I donāt know whether it is absolutely necessary for arith.t to avoid test.pl. But I do know that anyone refactoring tests needs to be careful and not assume that nothing is written in an odd way without a reason.
I would actually prefer that tests not be refactored at all\, but I am not in charge and am willing to live with it.
So Iām basically saying that I donāt want to be involved in refactoring tests\, but I want others to be aware of the danger.
(I know thatās not well written. I wrote it quickly. I probably repeated myself a few times.)
--
Father Chrysostomos
On 24 November 2012 03:12\, Father Chrysostomos via RT \perlbug\-followup@​perl\.org wrote:
On Fri Nov 23 13:24:27 2012\, jkeenan wrote:
On Mon Nov 19 08:47:17 2012\, sprout wrote:
I do think you are reading too much into what perlhack says. test.pl may be OK to use in op/\, but that does not mean that everything in there must use it.
Still\, separation into different directories does not sound like a bad idea.
I have created https://rt-archive.perl.org/perl5/Ticket/Display.html?id=115838 to address the general issue. Have you seen that yet?
I did. I have no comment on that in particular. Let me explain why.
I have seen test suite refactorings that basically sabotaged the tests\, so they were no longer testing what they were supposed to be testing. That actually happened in 5.10. A feature was added\, with tests. The tests were refactored. Later the feature was broken and 5.10 shipped with a broken feature\, but the tests still passed. So Iām paranoid.
I donāt fully understand or appreciate the reasons why certain things are done certain ways\, but I can appreciate that there may be reasons. So I donāt assume that all changes are safe to make.
I have seen similar with regex tests.
Yves
-- perl -Mre=debug -e "/just|another|perl|hacker/"
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-11-24 03:15]:
I have seen test suite refactorings that basically sabotaged the tests\, so they were no longer testing what they were supposed to be testing. That actually happened in 5.10. A feature was added\, with tests. The tests were refactored. Later the feature was broken and 5.10 shipped with a broken feature\, but the tests still passed. So Iām paranoid.
And anyone who has their epistemology down will now ask: but was the ineffectiveness of those tests a function of their refactoredness? You know that the tests tested failed to test for the right thing *after* being refactored\, but do you *also* know that they tested for the right thing *before* getting refactored? Put another way\, which came first: the refactoring or the brokenness? Just because evidence of brokenness followed evidence of refactoring doesnāt also mean brokenness followed refactoring. You cannot necessarily deduce causality from temporal sequence of observation\, even though our brains are strongly wired to work that way.
Itās so hard not to fool oneself\, to quote Feynman.
For making sure that refactorings donāt *introduce* brokenness if it was not previously present\, there is an easy if costly approach to reaching some level of confidence: just keep the old tests around and compare the shiny new testsā results with theirs\, for the duration of a couple of releases. At some point you can decide that youāve watched them spit out the same output for long enough to attain some X% confidence\, and can therefore delete the old crufty version.
But tests can never *prove* anything. Letās not forget that. In practice this is rarely relevant but it matters a great deal when for some reason you find yourself reasoning epistemologically about test suites.
(So for making sure brokenness doesnāt exist *in the first place*\, well\, good luck with thatā¦)
Regards\, -- Aristotle Pagaltzis // \<http://plasmasturm.org/>
On Sat Nov 24 04:43:32 2012\, aristotle wrote:
* Father Chrysostomos via RT \perlbug\-followup@​perl\.org [2012-11-24 03:15]:
I have seen test suite refactorings that basically sabotaged the tests\, so they were no longer testing what they were supposed to be testing. That actually happened in 5.10. A feature was added\, with tests. The tests were refactored. Later the feature was broken and 5.10 shipped with a broken feature\, but the tests still passed. So Iām paranoid.
And anyone who has their epistemology down will now ask: but was the ineffectiveness of those tests a function of their refactoredness?
Yes\, it was.
--
Father Chrysostomos
On Fri Nov 23 13:26:49 2012\, jkeenan wrote:
On Sun Nov 18 11:02:39 2012\, sprout wrote:
On Sun Nov 18 09:49:09 2012\, jkeen@verizon.net wrote:
The patch attached is the last of those begun at yesterday's St Louis Perl Hackathon. Thanks to all participants in that event.
The patch converts the file from using all hard-coded tests (or subroutines wrapping around hard-coded tests) to using functions pulled in by require-ing test.pl.
But test.pl uses infix + for its machinery\, and we are trying to test that here.
I am attaching a patch which *substitutes* for the patch previously attached to this RT. Father C\, I believe it should satisfy your concerns.
Please review (anyone\, not just Father C)\, as we had to write descriptions for 140 out of 167 individual tests in this file.
Thank you very much. Jim Keenan
Pushed to blead in commit 84721e9. Closing ticket.
Thank you very much. Jim Keenan
@jkeenan - Status changed from 'open' to 'resolved'
Migrated from rt.perl.org#115806 (status was 'resolved')
Searchable as RT115806$