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

Bail out if a class extends Test::Class::Moose with a nonexisting/bogus class #93

Closed
wants to merge 1 commit into from

Conversation

@HaraldJoerg
Copy link
Contributor

HaraldJoerg commented Jul 5, 2018

A module can extend another test module like this:
use Test::Class::Moose extends => 'My::Other::Test::Module';
With this patch, loading fails if My::Other::Test::Module either does not exist at all, or exists but is not a test module (i.e. does not use Test::Class::Moose itself) - instead of silently discarding the test class.
Some tests and a doc update are included.
Closes #70.

Copy link
Member

autarch left a comment

Thanks for working on this! I'd like to see some changes before I merge this.

@@ -0,0 +1,48 @@
#!/usr/bin/env perl

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

Tests should use strict & warnings. I updated all the test in master to start with that.

@@ -0,0 +1,48 @@
#!/usr/bin/env perl

use lib 'lib', 't/lib';

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

There's no need to use lib via use lib. That was in some other tests for some reason but I've now removed it in master.

# exception testing is not applicable. The modules get a directory of
# their own because they (intentionally) break TCM::Load.

use lib 't/badlib';

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

Please use all dirs in one use lib declaration.

@@ -112,12 +112,14 @@ sub expected_test_events {
event Ok => sub {
call pass => T();
call name =>
q{Overriding and calling parent, but we don't have a plan and parent does: 1},
q{Overriding and calling parent, but we don't have a plan and parent does: 1}

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

I've tidied all the code in master so this sort of thing shouldn't be necessary.


use lib 't/badlib';

my $error;

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

All of this code is very repetitive. I think you could make a sub that does most of this. Also, did you try using dies from Test2::V0? I think you could do something like:

is( dies { eval '...' }, q{}, 'Subclassing works fine' );
like( dies { eval '...' }, qr/.../, '...' );

This comment has been minimized.

@HaraldJoerg

HaraldJoerg Jul 7, 2018 Author Contributor

A sub can indeed do the job for the two tests which check the error message. However, I doubt about dies: by using eval, I prevent the code from dying, so dies will always return undef, and then restore $@ to its value before the call. I chose string eval because I need the use to happen at runtime, otherwise the tests can't catch it. Test2::V0 has a try_ok, but no try_fail. As an alternative, I could simulate use with require + import, which would then allow to use dies. But then, I'd always ask myself why I'm testing a routine import which the users aren't supposed to call...

I'm currently implementing all the other suggestions, update to come soon...ish (midnight is approaching).

my $error;

# Verify that the error isn't triggered for correct subclassing
eval <<EOE;

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

Always use some sort of quote with heredocs. In this case I think you can do eval <<'EOE';

@@ -159,6 +159,9 @@ sub import {
if ( my $parent = ( delete $args{parent} || delete $args{extends} ) ) {
my @parents = 'ARRAY' eq ref $parent ? @$parent : $parent;
$caller->meta->superclasses(@parents);
grep { $_->isa(__PACKAGE__) } @parents

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

I'd rather see this written as:

unless ( ... ) {
    ... # construct a $msg
    Carp::croak($msg);
}

I think the check itself can be simplified to unless ( $caller->isa(__PACKAGE__) ). After we set superclasses the ->isa method should do the right thing here.

The error message can be improved to give more information (and for grammar):

  • Show the entire inheritance chain using $caller->meta->linearized_isa.
  • If there is only one ancestor, say "The $caller class inherits from $ancestor, which does not inherit from " . __PACKAGE__
  • For two ancestors, "The $caller class has X and Y as ancestors but none of these classes inherit from " . __PACKAGE__
  • For more than two ancestors, "The $caller class has X, Y, and Z as ancestors but none of these classes inherit from " . __PACKAGE__
@@ -357,7 +360,9 @@ run does not match the plan.
=head2 Inheriting from another Test::Class::Moose class
List it as the C<extends> in the import list.
List it as the C<extends> in the import list. If the base class does

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

One space after a period.

use Test2::V0;

# This test verifies the error handling when a TCM class extends a
# non-TCM subclass. All this happens at compile time, so the usual

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

One space after a period.

# This test verifies the error handling when a TCM class extends a
# non-TCM subclass. All this happens at compile time, so the usual
# exception testing is not applicable. The modules get a directory of
# their own because they (intentionally) break TCM::Load.

This comment has been minimized.

@autarch

autarch Jul 7, 2018 Member

I'm not sure what TCM::Load has to do with anything. It's not being used in this test code.

@autarch
Copy link
Member

autarch commented Jul 7, 2018

Also, this needs an entry in the Changes file.

@HaraldJoerg
Copy link
Contributor Author

HaraldJoerg commented Jul 10, 2018

So, I hope I have implemented all your suggestions. I have factored out the grammar stuff to formulate the error message in an extra sub because I felt it would distract too much from the usual test workflow. I apologize for the messy git history: I guess I made some mistake and am now unable to clean up.

@autarch
Copy link
Member

autarch commented Aug 1, 2018

Something very weird is happening in this PR. The first three commits appear twice in a row for some weird reason. It might be easiest to make a new branch and cherry pick the commits.

Copy link
Member

autarch left a comment

See my last comment

@HaraldJoerg
Copy link
Contributor Author

HaraldJoerg commented Aug 1, 2018

I agree - the mess in this branch is too ugly. New PR coming up in a moment, against your current master branch.

@autarch
Copy link
Member

autarch commented Aug 1, 2018

No need to close this PR. Just make a new branch of the same name and force push.

@HaraldJoerg
Copy link
Contributor Author

HaraldJoerg commented Aug 2, 2018

Yes, thanks for the hint. I figured out that I could try a force push, and apparently it worked as intended. I had never done this for a PR-related branch before.

So, there's the problem with the Travis checks. I didn't see that issue in my tests with Test2::IPC version 1.302120, but get it after upgrading Test2::IPC to the CPAN version 1.302138 used by Travis.

A short research revealed that it is a known issue in exodist's module, and if I run the test with his git HEAD (July 17) then the warning disappears. So I guess this has to wait until Chad pushes to CPAN.

@autarch
Copy link
Member

autarch commented Aug 2, 2018

I merged this from the CLI with some tweaks. I'll wait for Travis to start passing again before a release.

@autarch autarch closed this Aug 2, 2018
autarch added a commit that referenced this pull request Aug 15, 2018
v0.93
  [ENHANCEMENTS]

  * The CLI class (and role) now allow you to pass a directory for the
  --classes option. This directory is searched recursively for classes to be
  loaded. Based on a feature request from jrubinator. GH #91.

  [BUG FIXES]

  * Extending a TCM class with a non-existing or non-TCM class now delivers a
    fatal error instead of silently ignoring the test class. Implemented by
    Harald Jörg. PR #93, fixes GH #70.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.