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

add new test to uncoverage standard dns function. #1806

Closed
wants to merge 5 commits into from

Conversation

marcosptf
Copy link
Contributor

was created a new function to dns standard module;

was created a new function to dns standard module;
@krakjoe
Copy link
Member

krakjoe commented Oct 17, 2016

Needs skipif for online tests.

@marcosptf
Copy link
Contributor Author

Hello @krakjoe how are you?
fine?
ill fix it asap!

thanks for your comment!

marcosptf - <marcosptf@yahoo.com.br> - @phpsp - sao paulo - br
--SKIPIF--
<?php
if (phpversion() < "5.3.0") {
Copy link
Member

Choose a reason for hiding this comment

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

Same as my comment to the hash tests, no version check needed here

<?php
if (phpversion() < "5.3.0") {
die('SKIP php version so lower.');
}
Copy link
Member

Choose a reason for hiding this comment

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

Like @krakjoe said, needs to test for online tests here

?>
--FILE--
<?php
$serverUrl = "yahoo.com";
Copy link
Member

Choose a reason for hiding this comment

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

Could you please use php.net here? Then we don't rely on external services for which we do not have any control over

@marcosptf
Copy link
Contributor Author

@krakjoe @KalleZ
i've fixed like you suggest!
is it correct?

--EXPECTF--
Warning: checkdnsrr() expects at least %d parameter, %i given in %s on line %d
NULL
bool(%s)
Copy link
Member

Choose a reason for hiding this comment

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

Are these values not reliable for bugs.php.net? If so then a baked in bool(true) or bool(false) should be fine? (I'm not entirely into how this function works entirely)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okey
i'll test it before in my computer and answer to you!
Thanks!!!

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, once you have done that then the test should be good enough to committed, thanks for your work!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KalleZ
Thank you about the explanation !!!

bool checkdnsrr ( string $host [, string $type = "MX" ] );
--CREDITS--
marcosptf - <marcosptf@yahoo.com.br> - @phpsp - sao paulo - br
--FILE--
Copy link
Member

Choose a reason for hiding this comment

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

This needs a SKIPIF section for online tests like @krakjoe mentioned:

if (getenv("SKIP_ONLINE_TESTS")) { die('skip: online test'); }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KalleZ
done!

@krakjoe
Copy link
Member

krakjoe commented Jan 3, 2017

Again, EXPECTF bool(%s) doesn't make sense, it is going to be bool whether the assertion is passing or failing.

@marcosptf
Copy link
Contributor Author

@krakjoe
okey
i'll fix it asap!
thanks for feedback

@marcosptf
Copy link
Contributor Author

Hi @krakjoe
fixed!!!

Thanks!!!!

@marcosptf
Copy link
Contributor Author

ping @krakjoe @KalleZ

@krakjoe
Copy link
Member

krakjoe commented Jun 1, 2017

Merged c7f45aa

Thanks.

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

Successfully merging this pull request may close these issues.

4 participants