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

Make tests pass with plain prove #92

Closed
wants to merge 3 commits into from

Conversation

@HaraldJoerg
Copy link
Contributor

HaraldJoerg commented Jul 5, 2018

When I run the tests from the repository with prove -vlbr, then two tests (basic.t and test_control_methods) have one failure each. The failure disappears when I directly call prove -vlbr t/basic.t. Apparently prove without a path prepends ./t/ before the test files: the failure reappears when I call prove -vlbr ./t/basic.t. The patch allows for an optional ./ before the path names in the die messages.
make test passes with and without the patch.

This PR is a "side effect" of my work with test-class-moose as my July assignment in the CPAN Pull Request Challenge.

Copy link
Member

autarch left a comment

Thanks for working on this. I think this can be simplified, however.

t/basic.t Outdated
@@ -227,7 +227,7 @@ subtest_streamed(
call subevents => array {
event Exception => sub {
call error => match
qr{\Qforced die at t\E.\Qbasic.t\E.+}s;
qr{\Qforced die at \E(\./)?\Qt\E.\Qbasic.t\E.+}s;

This comment has been minimized.

@autarch

autarch Jul 6, 2018 Member

The parens are unnecessary and I'm not sure this is the right fix anyway. I think it'd be better to rewrite the regex as something like this qr{\Qforced die at \E.*\Qbasic.t\E.+}s

We don't really care much about the path leading up to the filename, we just want to make sure it's reporting the right exception.

@@ -60,7 +60,7 @@ subtest_streamed(
};
event Diag => sub {
call message => match
qr/\Qforced die at t\E.\Qtest_control_methods.t\E.+/s;
qr/\Qforced die at \E(\.\/)?\Qt\E.\Qtest_control_methods.t\E.+/s;

This comment has been minimized.

@autarch

autarch Jul 6, 2018 Member

Same applies here.

@HaraldJoerg
Copy link
Contributor Author

HaraldJoerg commented Jul 6, 2018

Agreed, and adjusted accordingly.

@autarch
Copy link
Member

autarch commented Jul 7, 2018

Merged from the CLI. Thanks!

@autarch autarch closed this Jul 7, 2018
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.