test failures related to src/pmc/handle.pmc

[mikehh]

src/pmc/handle.pmc was added to trunk at r39472 by whiteknight++ - [io_rewiring] Merge the io_rewiring branch into trunk.

it caused test failures in manifest_tests and distro_tests.

kid51++ fixed the manifest_tests failures at r39509

this however resulted in other failures in codetest - some fixed by barney++ at r39610.

The following failures still remain:

distro_tests:

#   Failed test 'there are PMC files for all test files in t/pmc'
#   at t/distro/test_file_coverage.t line 57.
# files in src/pmc but not in test dir:
# 	handle
# Looks like you failed 1 test of 3.
t/distro/test_file_coverage.t .. 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 

codetest:

t/codingstd/pod_syntax.t ....... 2/2
#   Failed test 'Pod syntax correct'
#   at t/codingstd/pod_syntax.t line 45.
#          got: 'src/pmc/handle.pmc'
#     expected: ''
# You should use podchecker to check the failed files.
# Looks like you failed 1 test of 2.
t/codingstd/pod_syntax.t ....... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests

podchecker results in:

mhj@mhj-desktop:~/parrot$ podchecker src/pmc/handle.pmc
*** ERROR: =over on line 15 without closing =back at line EOF in file src/pmc/handle.pmc
src/pmc/handle.pmc has 1 pod syntax error.

As it stands at r39510 the file src/pmc/handle.pms is:

/*
Copyright (C) 2008, Parrot Foundation.
$Id: handle.pmc 39510 2009-06-11 12:51:08Z barney $

=head1 NAME

src/pmc/handle.pmc - IO Handle PMC

=head1 DESCRIPTION

This is the base-class for all IO-related PMCs.

=head2 Vtable Functions

=over 4

=cut

*/

#include "parrot/parrot.h"
#include "../src/io/io_private.h"

pmclass Handle provides Handle {
    /* TODO: Consider encapsulating PIOHANDLE as a PMC type, for subclassing */
    ATTR PIOHANDLE os_handle;         /* Low level OS descriptor      */

    VTABLE void init() {
        Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_INVALID_OPERATION,
            "Handle cannot be instantiated directly.");
    }

    VTABLE void init_pmc(PMC * init) {
        Parrot_ex_throw_from_c_args(interp, NULL, EXCEPTION_INVALID_OPERATION,
            "Handle cannot be instantiated directly.");
    }
}

/*
 * Local variables:
 *   c-file-style: "parrot"
 * End:
 * vim: expandtab shiftwidth=4:
 */

[jkeenan]

I fixed pod_syntax.t. I created a file t/pmc/handle.t to placate t/distro/test_file_coverage.t. There are no real tests there, but whiteknight is aware of the situation.

And, following discussion with Infinoid, I moved test_file_coverage.t to t/codingstd so that it gets run as part of make codetest, which gets run more frequently than make distro_tests.

The problems with make examples_tests are still outstanding.

kid51

[whiteknight]

I've been thinking about this issue since yesterday, and I think we're going about it all wrong. Instead of blindly following the test and creating a new t/pmc/handle.t test file, I say we fix the test to not require it.

Handle is intended to be an abstract type, and should not be instantiated and used directly. Instead, it is inherited by other IO-related PMC types to provide a common base. As such, it's already tested as well as it is going to be in t/pmc/filehandle.t and t/pmc/stringhandle.t, and others. A test file t/pmc/handle.t has nothing to do: You can't create a Handle directly, and even if you could it's not usable for anything by itself.

A far better solution to this issue is to create a blacklist of PMC types that do not need to have their own test file in t/pmc/*. This is how we can support the addition of more abstract PMC types in the future, if we choose to add more.

[jkeenan]

Replying to whiteknight:

I've been thinking about this issue since yesterday, and I think we're going about it all wrong. Instead of blindly following the test and creating a new t/pmc/handle.t test file, I say we fix the test to not require it. Handle is intended to be an abstract type, and should not be instantiated and used directly. Instead, it is inherited by other IO-related PMC types to provide a common base. As such, it's already tested as well as it is going to be

Whiteknight:

At the time I added t/pmc/handle.t I was unaware of the special nature of the Handle PMC. And since it was a recent addition, I figured that fixing the test was just another bit of cage cleaning such as is often required after a branch is merged into trunk. I was not previously aware of test_file_coverage.t or the coding standard policy which it implies and enforces. On IRC, Infinoid (IIRC) noted that its original placement in t/distro/ meant that it would typically get run only once a month as part of a release manager's pre-release run of make fulltest. So we discussed moving it to t/codingstd/ so that it would get run as part of the more often run make codetest. And then I wrote and committed t/pmc/handle.t.

Little did I know that that would prove to be the most controversial commit I've made to Parrot in more than a year! (See  discussion on list.)

Now, one response would be to revert my commit of t/pmc/handle.t so that test_file_coverge.t once again FAILs and so that the issue is not papered over. I don't think that would be a good idea three days before a release. Instead, I have opened a new TT, TT #759, which calls for a reconsideration of the 'each PMC must have its own test file' policy. I will leave to the release manager, i.e., you, whether to move on that reconsideration in the days before the release.

Thank you very much.
kid51

[whiteknight]

I fixed this test a while back. It does nothing but try to create a Handle and expects an exception to be thrown. We can discuss this issue in more detail later, but I think the main issue of this ticket has been addressed.