Skip to content

one more failing test for GH-14 #15

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

Closed

Conversation

vsespb
Copy link
Contributor

@vsespb vsespb commented Mar 19, 2014

GH-14 still not fixed, here is the test failing on my box (last newlines in file are important)

guillaumeaubert added a commit that referenced this pull request Mar 20, 2014
(GH-15) Missing "name" keyword prevented a test from working.
@guillaumeaubert
Copy link
Owner

Ah yes, this is now the same as https://github.com/guillaumeaubert/Perl-Critic-Policy-ValuesAndExpressions-PreventSQLInjection/blob/master/t/ValuesAndExpressions/PreventSQLInjection.run#L116, thanks to the fix in eef8806. My apologies for the confusion, I wish I had noticed those typos in the closing terminators earlier.

Would you mind testing with the latest pull of the master branch? If you confirm that this is fixed now, I will then close this ticket.

@vsespb
Copy link
Contributor Author

vsespb commented Mar 20, 2014

I tested, LGTM (apart from PR 17).

Note that I am not good PPI tester, I just tried to include test case for your pop(). I.e. if you have pop @heredoc - there should be test failing without it. And when I tried to hack it I discovered couple of edge cases.

@guillaumeaubert
Copy link
Owner

I tested, LGTM (apart from PR 17).

Great, I'm going to close GH-15 without merging then, and keep GH-17 open instead.

Note that I am not good PPI tester, I just tried to include test case for your pop(). I.e. if you have pop @heredoc - there should be test failing without it. And when I tried to hack it I discovered couple of edge cases.

Thank you so much for all the testing you've done! You've found a lot of edge cases, and the test cases you're adding are definitely helping me to make this module more reliable.

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

Successfully merging this pull request may close these issues.

2 participants