test to bzflush function

[marcosptf]

was create a test to bzflush function, where there dont exist anyone yet.

[smalyshev]

Not sure this test actually tests that the function works - it only tests that it returns true.

[marcosptf]

Hi @smalyshev,
h r u ?
fine?

i see it and i know that you are all right!
buuuuttt, i've search in repository and dont found any source code about this function, for example:

a)in this file, don't have source code about this function, just calls and callback
https://github.com/php/php-src/blob/250938e2d35fc54161a18167b7901c5e3b574371/ext/bz2/bz2.c

b)this function is not coverage, yet

c) i don't found search code about

d) here explain that this function just return false, if there are some problem or failure, but beleve me, i've try force a fail test case but, just return true, because it work so fine

e) there, i don't see any more case of test to do, if has, please, give me some examples!

f)thanks for everything

g)hugs from brazil php community

:-)

[jerrygrey]

I agree with @smalyshev, this isn't actually testing what the function should be doing. Due to what the function does, it is hard to test, but this is the way I would go about it:

--FILE--
<?php
$file = __DIR__ . DIRECTORY_SEPARATOR . 'bzflush_test.txt.bz2';
$text = "This is a test string.";

$bz1 = bzopen($file, 'w');
bzwrite($bz1, $text);
var_dump(bzflush($bz1));

$bz2 = bzopen($file, 'r');
rewind($bz2);
var_dump(bzread($bz2));
bzclose($bz2);

bzclose($bz1);
?>
--CLEAN--
<?php 
unlink(__DIR__ . DIRECTORY_SEPARATOR . 'bzflush_test.txt.bz2'); 
?>
--EXPECTF--
bool(true)
string(%d) "This is a test string."

[marcosptf]

@jerrygrey
i did the test on php console like the example that your suggest:

[jerrygrey]

@marcosptf I didn't test the code beforehand as it was meant as just an idea on how you could go about testing this function. I have edited the code above, but it looks like it found a bug in the function to me.

Edit: Yep, it is a bug, see here. I added the rewind() in, but to me the second bzopen should automatically do this as per fopen() documentation for "r" mode: "Open for reading only; place the file pointer at the beginning of the file." To me this is something that should be looked at.

Edit 2: I have filed a bug report for it, #70819.

[marcosptf]

Hi @jerrygrey !
then it confirm that the function bzflush(); work fine like my test show, okey ?

i did another debugs test without bzflush(); , the behavior is the same ==>

[jerrygrey]

@marcosptf I have edited my code to avoid that bug, the test I wrote should work now without any issues. The bug report I filed as nothing to do with the bzflush() function, and as I said I have edited my code above so the bug doesn't affect the test for the function.

The two tests you did proves nothing, just because a function returns true doesn't mean it is doing what it should be doing, and the point of the bzflush() function is to flush the buffer, when you use bzwrite() the data doesn't go straight into the file, it is buffered in memory by PHP until it reaches a defined size (see stream_set_write_buffer()) or the functions bzflush() or bzclose() are used. This is why your latest test is what you'd expect, i.e. the file being empty after using bzwrite().

These are just my comments though, it is up to you what you do with this PR.

[marcosptf]

"The two tests you did proves nothing....."
uhasuhuahsuhauhsuhaushuahs
kkkkkkkkkkkkkkkkkkkkkkkkkk
:D :D :D :D :D :D :D :D :D

nothing?
we've found a new (old) bug with 12 year old!!!
but okey, i understand what you said
i'll try again do a new test to this case like

thanks @jerrygrey

👍

[jerrygrey]

As pointed out by reeze, this function doesn't actually do anything, see here. It doesn't need a test.

[marcosptf]

ooohh noooo!!!!!!!

[webysther]

Well, maybe check if return 0. If return another value in future the function was changed, point to make a complete test.

[webysther]

@marcosptf sad but fun. Kkkkkk

[marcosptf]

kkkkk

[krakjoe]

@marcosptf This test is still wrong; it unconditionally attempt to flush what may have failed to open, and then prints that it couldn't open a file if it fails to flush.

Please fix the test so that it adds value, and is correct, simply adding tests so that coverage is reported to be higher is not a useful thing to do, as I'm sure you understand by now :)

[marcosptf]

okey @krakjoe
you are totally right!!!
i understand now!

thanks for explain !!!

[marcosptf]

hi @krakjoe

ive fixed this code like you said.
the code before fix =>
57876cd

Thanks!
have a nice day!

[krakjoe]

Sorry, but having read the source code of extension before merging: This function is an alias of fflush, and that function is well tested.

Another user did already mention this before, it's a comment in the bug linked in this thread.

Adding the test just so that test coverage is increased is simply not valuable, so I'm closing this.