Navigation Menu

Skip to content
This repository has been archived by the owner on Mar 7, 2019. It is now read-only.

skip inline* tests when Acme::Alien::DontPanic has an obviously bad dist_dir #108

Merged
merged 3 commits into from Feb 21, 2015
Merged

skip inline* tests when Acme::Alien::DontPanic has an obviously bad dist_dir #108

merged 3 commits into from Feb 21, 2015

Conversation

plicease
Copy link
Contributor

This would probably have fixed this test failure:

http://ppm4.activestate.com/x86_64-linux/5.20/2000/P/PL/PLICEASE/Alien-Base-0.010.d/log20150217T080630.txt

Looks like it is trying to use a blib install gone bad. Isolating the problem with Acme::Alien::DontPanic would ultimately be good, but I don't want that to block a working Alien::Base install.

@jberger
Copy link
Member

jberger commented Feb 20, 2015

The link doesn't work for me. And yeah, not having one of the dummy modules should not block a (working!) installation of AB, but then again, how do we test that it is working without them?

@plicease
Copy link
Contributor Author

That's weird the link isn't working for me now either.

@plicease
Copy link
Contributor Author

@plicease
Copy link
Contributor Author

@jberger Agree with your comment but I think that the inline* tests do their bests to ensure the inline integration works, but at the same time not break real life installs. This PR just makes the testing a little more complete. We could make them developer only tests, but then we loose any feedback from cpantesters.

@zmughal
Copy link
Member

zmughal commented Feb 20, 2015

This makes sense, but it seems a little too specific.

How about having an invalid dist_dir throw an exception when you try to use Acme::Alien::DontPanic? Can this logic be applied to any child class of Alien::Base? Since the dist_dir is used for anything with a share install_type, it would be good to have an indication that something went wrong early on.

@plicease
Copy link
Contributor Author

@zmughal Good suggestion. In fact just reading your response reminded me that Acme::Alien::DontPanic can be installed as install_type=system ... in fact it does this in travis as one of its permutations.

@plicease
Copy link
Contributor Author

Just to explain if it isn't clear File::ShareDir::dist_dir already croaks with that message if the directory isn't there. I've just updated it so that it also dies if not finished_installing and working_directory is not there. Then calling it in void context in import will throw the exception as discussed above.

@@ -114,6 +114,8 @@ sub import {
my $class = shift;

return if $class->install_type('system');

$class->dist_dir;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a comment on why this line is called here (e.g., "Sanity check in order to ensure that dist_dir can be found. This will throw an exception otherwise.")

@zmughal
Copy link
Member

zmughal commented Feb 20, 2015

Other than my comment (just for documentation purposes), this looks good to me.

Merge: Aye

@mohawk2
Copy link
Contributor

mohawk2 commented Feb 21, 2015

Likewise: please add that comment. Then: aye.

@plicease plicease merged commit 1cb7c73 into Perl5-Alien:master Feb 21, 2015
@plicease plicease deleted the skip_inline_and_inline_cpp_tests_on_bad_acme branch February 21, 2015 11:58
@plicease
Copy link
Contributor Author

This is on its way to CPAN as 0.010_01. I expect to do a production release in a day or two.

plicease added a commit that referenced this pull request Jul 17, 2017
This should address #108, and also provide a useful diagnostic in other situations
plicease added a commit that referenced this pull request Jul 17, 2017
This should address #108, and also provide a useful diagnostic in other situations
plicease added a commit that referenced this pull request Jul 17, 2017
This should address #108, and also provide a useful diagnostic in other situations
plicease added a commit that referenced this pull request Jul 17, 2017
This should address #108, and also provide a useful diagnostic in other situations
plicease added a commit that referenced this pull request Jul 17, 2017
This should address #108, and also provide a useful diagnostic in other situations
plicease added a commit that referenced this pull request Jul 17, 2017
Alien-Base: This should address #108, and also provide a useful diagnostic in other situations
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants