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

Test coverage #40

Merged
merged 5 commits into from Jan 10, 2017

Conversation

Projects
None yet
2 participants
@openstrike
Contributor

openstrike commented May 18, 2015

Here are some additional tests to improve the coverage. Please be aware that I do not have access to any MSWin32 systems so have not been able to test the changes on that platform.

These additional tests have shown up one thing which you might consider a bug: If the user does not supply a port to check_port() or to wait_port() (which the POD describes as being mandatory) the code does not test this but passes the undef along to IO::Socket::IP->new() which then throws the exception "Expected 'PeerService'". A couple of my new tests look for this error but you might consider it to be better if check_port() and wait_port() would check that a port has been supplied and carp something appropriate otherwise.

I hope you find this PR useful. The work has been done as part of the CPAN PR Challenge.

@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Jan 6, 2017

Contributor

I'd be interested in any feedback you might have on this PR - are any changes required prior to merging?

Contributor

openstrike commented Jan 6, 2017

I'd be interested in any feedback you might have on this PR - are any changes required prior to merging?

@syohex

This comment has been minimized.

Show comment
Hide comment
@syohex

syohex Jan 7, 2017

Collaborator
  • This PR cannot be merged with current master branch. Conflicts should be fixed.
  • Please don't use tab for indentation.
Collaborator

syohex commented Jan 7, 2017

  • This PR cannot be merged with current master branch. Conflicts should be fixed.
  • Please don't use tab for indentation.
@openstrike

This comment has been minimized.

Show comment
Hide comment
@openstrike

openstrike Jan 8, 2017

Contributor

Thanks for the feedback. I've removed what tabs I could see and resolved the merge conflicts. If there's anything else you need for this PR do let me know.

Contributor

openstrike commented Jan 8, 2017

Thanks for the feedback. I've removed what tabs I could see and resolved the merge conflicts. If there's anything else you need for this PR do let me know.

@syohex syohex merged commit f0b23f4 into tokuhirom:master Jan 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment