Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discrepancy between Porting/perltodo.pod and pod/perlhack.pod re t/cmd/ #12752

Closed
p5pRT opened this issue Feb 2, 2013 · 13 comments
Closed

Discrepancy between Porting/perltodo.pod and pod/perlhack.pod re t/cmd/ #12752

p5pRT opened this issue Feb 2, 2013 · 13 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 2, 2013

Migrated from rt.perl.org#116615 (status was 'resolved')

Searchable as RT116615$

@p5pRT
Copy link
Author

p5pRT commented Feb 2, 2013

From @jkeenan

There is a discrepancy between Porting/perltodo.pod and pod/perlhack.pod
with respect to whether tests found in directory t/cmd/ can use
t/test.pl or not.

Porting/perltodo.pod says​:

#####
  Migrate t/ from custom TAP generation
  Many tests below t/ still generate TAP by "hand", rather
  than using library functions. As explained in "TESTING" in
  perlhack, tests in t/ are written in a particular way to
  test that more complex constructions actually work before
  using them routinely. Hence they don't use "Test​::More",
  but instead there is an intentionally simpler library,
  t/test.pl. However, quite a few tests in t/ have not been
  refactored to use it. Refactoring any of these tests, one
  at a time, is a useful thing TODO.

  The subdirectories base, cmd and comp, that contain the
  most basic tests, should be excluded from this task.
#####

pod/perlhack.pod says​:

#####
  * t/base, t/comp and t/opbasic

  Since we don't know if require works, or even
  subroutines, use ad hoc tests for these three. Step
  carefully to avoid using the feature being tested.
  Tests in t/opbasic, for instance, have been placed
  there rather than in t/op because they test
  functionality which t/test.pl presumes has already
  been demonstrated to work.

  * t/cmd, t/run, t/io and t/op

  Now that basic require() and subroutines are tested,
  you can use the t/test.pl library.
######

In point of fact, there are two test files in t/cmd/ which "require
t/test.pl"​:

#####
$ ack require t/cmd/
t/cmd/lexsub.t
6​: require './test.pl';

t/cmd/while.t
5​: require "test.pl";
#####

There aren't that many files in t/cmd/ to begin with​:

#####
$ ls t/cmd
elsif.t for.t lexsub.t mod.t subval.t switch.t while.t
######

We need to investigate this discrepancy. Are the files *not* currently
using t/test.pl capable of using it? Or, perhaps, are the files
currently using t/test.pl incorrectly using it?

Once we make a determination, then updating the tests becomes something
which only requires ordinary Perl experience and could become a task at
a hackathon.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Feb 18, 2013

From @nwc10

On Sat, Feb 02, 2013 at 03​:34​:37PM -0800, James E Keenan wrote​:

There is a discrepancy between Porting/perltodo.pod and pod/perlhack.pod
with respect to whether tests found in directory t/cmd/ can use
t/test.pl or not.

Well spotted.

In point of fact, there are two test files in t/cmd/ which "require
t/test.pl"​:

#####
$ ack require t/cmd/
t/cmd/lexsub.t
6​: require './test.pl';

t/cmd/while.t
5​: require "test.pl";
#####

There aren't that many files in t/cmd/ to begin with​:

#####
$ ls t/cmd
elsif.t for.t lexsub.t mod.t subval.t switch.t while.t
######

We need to investigate this discrepancy. Are the files *not* currently
using t/test.pl capable of using it? Or, perhaps, are the files
currently using t/test.pl incorrectly using it?

lexsub.t definitely can go "later" (ie t/op/) because it's testing something
that isn't used by any part of the minitest infrastructure (ie t/TEST or
t/test.pl)

I think that while.t can also go later, because (a) it's testing more
advanced features of while and (b) there's a t/base/while.t to check that
the really simple stuff works.

So I'd suggest that both of these two belong in t/op

I think that t/cmd/for.t can be split - everything starting from
"# A lot of tests to check that reversed for works." could go into a
t/op/for.t, and use test.pl

I think that all the others are things that really should be working
before t/test.pl is used, so should stay there, and not use test.pl
Which therefore also means that t/porting/test_bootstrap.pl should be
patched to also check t/cmd for (naughty) use of t/test.pl

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Feb 18, 2013

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Mar 2, 2013

From itcharlie@gmail.com

This patch was prepared at the NY Perl Hackaton by Charlie Gonzalez and
Mottaqui Karim.

Please review.

On Sat Feb 02 15​:34​:36 2013, jkeen@​verizon.net wrote​:

There is a discrepancy between Porting/perltodo.pod and pod/perlhack.pod
with respect to whether tests found in directory t/cmd/ can use
t/test.pl or not.

Porting/perltodo.pod says​:

#####
Migrate t/ from custom TAP generation
Many tests below t/ still generate TAP by "hand", rather
than using library functions. As explained in "TESTING" in
perlhack, tests in t/ are written in a particular way to
test that more complex constructions actually work before
using them routinely. Hence they don't use "Test​::More",
but instead there is an intentionally simpler library,
t/test.pl. However, quite a few tests in t/ have not been
refactored to use it. Refactoring any of these tests, one
at a time, is a useful thing TODO.

The subdirectories base, cmd and comp, that contain the
most basic tests, should be excluded from this task.
#####

pod/perlhack.pod says​:

#####
* t/base, t/comp and t/opbasic

   Since we don't know if require works\, or even
   subroutines\, use ad hoc tests for these three\. Step
   carefully to avoid using the feature being tested\.
   Tests in t/opbasic\, for instance\, have been placed
   there rather than in t/op because they test
   functionality which t/test\.pl presumes has already
   been demonstrated to work\.

* t/cmd, t/run, t/io and t/op

   Now that basic require\(\) and subroutines are tested\,
   you can use the t/test\.pl library\.

######

In point of fact, there are two test files in t/cmd/ which "require
t/test.pl"​:

#####
$ ack require t/cmd/
t/cmd/lexsub.t
6​: require './test.pl';

t/cmd/while.t
5​: require "test.pl";
#####

There aren't that many files in t/cmd/ to begin with​:

#####
$ ls t/cmd
elsif.t for.t lexsub.t mod.t subval.t switch.t while.t
######

We need to investigate this discrepancy. Are the files *not* currently
using t/test.pl capable of using it? Or, perhaps, are the files
currently using t/test.pl incorrectly using it?

Once we make a determination, then updating the tests becomes something
which only requires ordinary Perl experience and could become a task at
a hackathon.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Mar 2, 2013

@p5pRT
Copy link
Author

p5pRT commented Mar 3, 2013

From @jkeenan

On Sat Mar 02 13​:53​:37 2013, itcharlie wrote​:

This patch was prepared at the NY Perl Hackaton by Charlie Gonzalez and
Mottaqui Karim.

Please review.

The patch attached updates the MANIFEST and addresses one issue which we
didn't fully understand when discussing it yesterday at the hackathon.
The patch should be applied after the patch by Mottaqui and Charlie.

Nick, I think this follows the roadmap you set out. Agreed?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Mar 5, 2013

From @nwc10

On Sun, Mar 03, 2013 at 05​:35​:28AM -0800, James E Keenan via RT wrote​:

On Sat Mar 02 13​:53​:37 2013, itcharlie wrote​:

This patch was prepared at the NY Perl Hackaton by Charlie Gonzalez and
Mottaqui Karim.

Please review.

The patch attached updates the MANIFEST and addresses one issue which we
didn't fully understand when discussing it yesterday at the hackathon.
The patch should be applied after the patch by Mottaqui and Charlie.

You didn't attach the patch

Also, I feel that the *whole* change should be in one patch, as it all
belongs together.

The e-mail address on their patch is taqqui@​sandbox03.nj1.rubensteintech.com

a) is that correct? and desired?
b) that address isn't in AUTHORS

I tend to put the new address in AUTHORS as the commit before the new author's
commit, so that tests don't fail at any revision.

Nick, I think this follows the roadmap you set out. Agreed?

Yes. It seems to be all there apart from the housekeeping.
Thanks.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Mar 5, 2013

From @nwc10

On Tue, Mar 05, 2013 at 02​:21​:12PM +0000, Nicholas Clark wrote​:

Yes. It seems to be all there apart from the housekeeping.
Thanks.

I forgot to add. Due to a bug fix patching t/cmd/for.t in commit
89e006a it won't apply cleanly to blead.

but I found that I can apply it to 89e006a^ and then rebase onto
blead - git rebase seems to be able to cope with moving the edited file.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Mar 5, 2013

From @nwc10

On Tue, Mar 05, 2013 at 02​:21​:12PM +0000, Nicholas Clark wrote​:

On Sun, Mar 03, 2013 at 05​:35​:28AM -0800, James E Keenan via RT wrote​:

The patch attached updates the MANIFEST and addresses one issue which we
didn't fully understand when discussing it yesterday at the hackathon.
The patch should be applied after the patch by Mottaqui and Charlie.

You didn't attach the patch

Also, I feel that the *whole* change should be in one patch, as it all
belongs together.

Actually, forgot to say, it *has* to be in one patch.
If a commit deletes files and don't update the MANIFEST, then if attempting
to build from clean, Configure will bail out.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Mar 6, 2013

From @jkeenan

On Tue Mar 05 06​:39​:09 2013, nicholas wrote​:

On Tue, Mar 05, 2013 at 02​:21​:12PM +0000, Nicholas Clark wrote​:

On Sun, Mar 03, 2013 at 05​:35​:28AM -0800, James E Keenan via RT wrote​:

The patch attached updates the MANIFEST and addresses one issue
which we
didn't fully understand when discussing it yesterday at the
hackathon.
The patch should be applied after the patch by Mottaqui and Charlie.

You didn't attach the patch

Also, I feel that the *whole* change should be in one patch, as it all
belongs together.

Actually, forgot to say, it *has* to be in one patch.
If a commit deletes files and don't update the MANIFEST, then if
attempting
to build from clean, Configure will bail out.

Nicholas Clark

We will straighten out the housekeeping and submit a new patch.

In the meantime, how does the code look?

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Mar 6, 2013

From @nwc10

On Tue, Mar 05, 2013 at 07​:46​:13PM -0800, James E Keenan via RT wrote​:

We will straighten out the housekeeping and submit a new patch.

In the meantime, how does the code look?

Well, the moved files are byte-for-byte perfect (assuming that the patch is
rebased onto blead) so there's not much to fault about that :-)

I think that the commented out code in t/op/for.t should go. Things like
# my $test = 0;

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Mar 14, 2013

From @jkeenan

On Wed Mar 06 00​:19​:17 2013, nicholas wrote​:

On Tue, Mar 05, 2013 at 07​:46​:13PM -0800, James E Keenan via RT wrote​:

We will straighten out the housekeeping and submit a new patch.

In the meantime, how does the code look?

Well, the moved files are byte-for-byte perfect (assuming that the
patch is
rebased onto blead) so there's not much to fault about that :-)

I think that the commented out code in t/op/for.t should go. Things like
# my $test = 0;

Nicholas Clark

Previous patch, plus one additional correction from one of the hackathon
participants, applied in commits
e150f77 and
0a395aa. Closing ticket.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Mar 14, 2013

@jkeenan - Status changed from 'open' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant