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

test to function zlib_get_coding_type(void); #1815

Closed
wants to merge 5 commits into from

Conversation

marcosptf
Copy link
Contributor

was add new coverage test to module zlib;

was add new coverage test to module zlib;
@smalyshev
Copy link
Contributor

This test doesn't seem to be very useful if it's always returning false.

@nikic
Copy link
Member

nikic commented Sep 5, 2016

Looks like this function is supposed to be used in conjunction with zlib.output_compression=On, in which case it depends on the Accept-Encoding header we receive.

@marcosptf
Copy link
Contributor Author

marcosptf commented Sep 6, 2016

@smalyshev
@nikic
okey folks!!
ill fix it asap

thanks for your comments!!!

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.

We do not even support PHP 5.3, so this SKIPIF should not be there. Active branches are: 5.6, 7.0, 7.1 and master (7.2)

@marcosptf
Copy link
Contributor Author

@smalyshev
@nikic
@KalleZ

Hi PHP Masters!!!
how are you? fine?

i've fixed this code like you suggest, its correct ?

Thanks

print($codingType);
?>
--CLEAN--
<?php
Copy link
Member

Choose a reason for hiding this comment

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

The --CLEAN-- section is not needed for variable unsets, its a separate generated file after the phpt is executed. It is commonly used to remove temporary files and similar

--CREDITS--
marcosptf - <marcosptf@yahoo.com.br> - @phpsp - sao paulo - br
--SKIPIF--
<?php
Copy link
Member

Choose a reason for hiding this comment

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

zlib_get_coding_type() is available as of PHP 4.3.2, so the --SKIPIF-- section should be changed to reflect whether or not the zlib extension is loaded e.g.:

if (!extension_loaded('zlib')) {
print 'skip: zlib extension not available';
}

or similar

$compressionCoding = zlib_get_coding_type();
$codingType = "get zlib type error";

if((!$compressionCoding) || ("gzip" == $compressionCoding) || ("deflate" == $compressionCoding)){
Copy link
Member

Choose a reason for hiding this comment

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

It seems a little odd to check for all 3 return values here? Since the suite is executed on CLI, then this value will always be false unless otherwise defined by a HTTP request header which do not occur when using make test (and zlib.compression must be set to On in php.ini).

Since make test only supports --POST-- variables to be sent, but not modifications of $_SERVER variables, meaning we cannot really get a meaningful value of testing this function unless we somehow can modify $_SERVER. The zlib extension will initialize ZLIBG(compression_coding) at RINIT, which zlib_get_coding_type() uses, meaning we cannot override $_SERVER before calling zlib_get_coding_type(), so the result will always be false.

@KalleZ
Copy link
Member

KalleZ commented Sep 27, 2016

I added some inline comments. The TL;DR version is that I don't think it makes much sense since the return value we get is always false, and only really makes sense to commit if we are going for gcov coverage

@marcosptf
Copy link
Contributor Author

marcosptf commented Sep 27, 2016

@KalleZ thanks for your comments
like this?
5b458a0

if (phpversion() < "5.6.0") {
die('SKIP php version so lower.');
}
if(!extension_loaded('zlib')) die('skip, zlib not loader');
Copy link
Contributor

Choose a reason for hiding this comment

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

ITYM not loaded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smalyshev
fixed!

@krakjoe
Copy link
Member

krakjoe commented Jan 3, 2017

Consensus seems to be that this is not a worth while test, so closing this PR.

If the test can be written to add value, please take this action as encouragement to do so.

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.

5 participants