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

Ported some test cases to Test::More #25

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@upasana-me
Contributor

upasana-me commented Jan 18, 2015

Hi,
I have ported some test cases to use Test::More & some minor indentation fixes for better readability.

Thanks.

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

Please use is not like to replicate the exact match it was before. Otherwise this would match at foo-two-bar too which it should not. Please check also with the rest of the code.

noxxi commented on t/alpn.t in d68edb9 Jan 19, 2015

Please use is not like to replicate the exact match it was before. Otherwise this would match at foo-two-bar too which it should not. Please check also with the rest of the code.

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

This commit replaces all likes (except one) with is : 63aea7f
One like which I have not replaced with "is" is this :
t/core.t:98: like( $cert, qr/IO::Socket::SSL Demo CA/, "Client Verify-sub Check");

Owner

upasana-me replied Jan 21, 2015

This commit replaces all likes (except one) with is : 63aea7f
One like which I have not replaced with "is" is this :
t/core.t:98: like( $cert, qr/IO::Socket::SSL Demo CA/, "Client Verify-sub Check");

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

The following call of ok will check $server. Better use this and exit after the ok call if !$server.

noxxi commented on t/auto_verify_hostname.t in d68edb9 Jan 19, 2015

The following call of ok will check $server. Better use this and exit after the ok call if !$server.

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

This commit addresses it 11c6e20

Owner

upasana-me replied Jan 21, 2015

This commit addresses it 11c6e20

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

together with the next ok this would print not not ... on failure which is not the intended behavior. Better just remove this print and use only ok.

noxxi commented on t/auto_verify_hostname.t in d68edb9 Jan 19, 2015

together with the next ok this would print not not ... on failure which is not the intended behavior. Better just remove this print and use only ok.

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

This commit addresses it : 11c6e20

Owner

upasana-me replied Jan 21, 2015

This commit addresses it : 11c6e20

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

testlib is in nearly all tests needed because it checks for support for fork and skips test if not available. Please add it back everywhere.

noxxi commented on t/alpn.t in d68edb9 Jan 19, 2015

testlib is in nearly all tests needed because it checks for support for fork and skips test if not available. Please add it back everywhere.

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

This commit adds testlib.pl back to all the tests : 512657f

Owner

upasana-me replied Jan 21, 2015

This commit adds testlib.pl back to all the tests : 512657f

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

Better use pass instead of ok(1,.. which matches also the fail you used above. Please check also the rest of the code for the same issue.

noxxi commented on t/cert_no_file.t in d68edb9 Jan 19, 2015

Better use pass instead of ok(1,.. which matches also the fail you used above. Please check also the rest of the code for the same issue.

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

This commit uses pass in place of ok(1,...) : 9273efc

Owner

upasana-me replied Jan 21, 2015

This commit uses pass in place of ok(1,...) : 9273efc

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

I think you can call plan skip_all only in subtests or before you did another plan. Otherwise you get You tried to plan twice.... Please check also the rest of the code.

noxxi commented on t/cert_no_file.t in d68edb9 Jan 19, 2015

I think you can call plan skip_all only in subtests or before you did another plan. Otherwise you get You tried to plan twice.... Please check also the rest of the code.

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

I am not getting any such error in any of the tests, on my debian system, except t/alpn.t
I can't run alpn.t on my system, because of no alpn support on my system, so not sure if that test will give any errors when there would be alpn support on the system.

Owner

upasana-me replied Jan 21, 2015

I am not getting any such error in any of the tests, on my debian system, except t/alpn.t
I can't run alpn.t on my system, because of no alpn support on my system, so not sure if that test will give any errors when there would be alpn support on the system.

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

I can not see where the additional test comes from. Could you point me to the relevant place?

noxxi commented on t/core.t in d68edb9 Jan 19, 2015

I can not see where the additional test comes from. Could you point me to the relevant place?

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 19, 2015

Owner

I wrote one additional test, but now I realized that it was not really required, so I will change this number.

Owner

upasana-me replied Jan 19, 2015

I wrote one additional test, but now I realized that it was not really required, so I will change this number.

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

numtests set back to 40 : 478912e

Owner

upasana-me replied Jan 21, 2015

numtests set back to 40 : 478912e

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

I actually like array[0] as it was before more than array[ 0 ]. Could you please change this and the following cases back?

noxxi commented on t/core.t in d68edb9 Jan 19, 2015

I actually like array[0] as it was before more than array[ 0 ]. Could you please change this and the following cases back?

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 19, 2015

Owner

Sorry, but I have not understood this comment. Do you mean that I should not replace tests which have $array[ 0 ] with is?

Owner

upasana-me replied Jan 19, 2015

Sorry, but I have not understood this comment. Do you mean that I should not replace tests which have $array[ 0 ] with is?

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

No, I just did not like the spaces in array[<space>0<space>], i.e. I like array[0] more than array[ 0 ].

noxxi replied Jan 19, 2015

No, I just did not like the spaces in array[<space>0<space>], i.e. I like array[0] more than array[ 0 ].

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 19, 2015

Owner

Alright,, thanks, I will remove spaces!

Owner

upasana-me replied Jan 19, 2015

Alright,, thanks, I will remove spaces!

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

All $array[ x ] are used in like, so this commit 63aea7f

Owner

upasana-me replied Jan 21, 2015

All $array[ x ] are used in like, so this commit 63aea7f

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 19, 2015

Many thanks for you work. But there are some issues which prevent the direct commit for now (see comments).

Regards,
Steffen

noxxi commented on d68edb9 Jan 19, 2015

Many thanks for you work. But there are some issues which prevent the direct commit for now (see comments).

Regards,
Steffen

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 19, 2015

Owner

Thanks so much for giving your time to review this PR, I will push the changes soon.

Owner

upasana-me replied Jan 19, 2015

Thanks so much for giving your time to review this PR, I will push the changes soon.

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 21, 2015

Owner

I have made the changes. Could you please review it again?

Thanks.

Owner

upasana-me replied Jan 21, 2015

I have made the changes. Could you please review it again?

Thanks.

@noxxi

This comment has been minimized.

Show comment
Hide comment
@noxxi

noxxi Jan 26, 2015

Owner

Sorry for the delay.
I've merged your changes into the main branch, but as a single commit. After this I had to made some changes:

  • some indentation and white space fixes in 8cf2973
  • replace skip_all with fail+exit in various places, e79e825. skip_all returns true for the test which is not the intended behavior in these cases.
  • I had to revert the use of Test::More in alpn.t, c1af848. It looks like Test::More does not work if both parent and child process inside a test issue messages.

Thanks again for helping on the project.
Regards,
Steffen

Owner

noxxi commented Jan 26, 2015

Sorry for the delay.
I've merged your changes into the main branch, but as a single commit. After this I had to made some changes:

  • some indentation and white space fixes in 8cf2973
  • replace skip_all with fail+exit in various places, e79e825. skip_all returns true for the test which is not the intended behavior in these cases.
  • I had to revert the use of Test::More in alpn.t, c1af848. It looks like Test::More does not work if both parent and child process inside a test issue messages.

Thanks again for helping on the project.
Regards,
Steffen

@noxxi noxxi closed this Jan 26, 2015

@upasana-me

This comment has been minimized.

Show comment
Hide comment
@upasana-me

upasana-me Jan 27, 2015

Contributor

Thanks so much for merging my PR & sorry for the inconvenience caused.

Contributor

upasana-me commented Jan 27, 2015

Thanks so much for merging my PR & sorry for the inconvenience caused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment