-
Notifications
You must be signed in to change notification settings - Fork 502
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
DecompressionTest: improve tests #595
Conversation
'compressed' => "\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x03\x4b\xcb\xcf\x4f\x4a" | ||
. "\x2c\x02\x00\x95\x1f\xf6\x9e\x06\x00\x00\x00", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm currently trying to figure out how we can cross-test these compressed strings.
The closest I've gotten so far:
echo 'foobar' | gzip -c | hexdump -v -e '"\\x" 1/1 "%02x"' ; echo
This produces the following output:
\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x03\x4b\xcb\xcf\x4f\x4a\x2c\xe2\x02\x00\x47\x97\x2c\xb2\x07\x00\x00\x00
It looks like the output formatting is correct, but the shell gzip
produces a different result. Not sure yet whether this is due to the input, or the compression algorithm...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of places in the tests where we will need to do a cross-check that the test data is correct.
IIRC, I already did so for the IdnaEncoderTest and documented the tooling I used inline in the test, but there are more (like this one).
Maybe we should open a separate issue about this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: a number of these test cases were only recently added via #309 by @soulseekah . Maybe they can shed some light on how the test cases were created ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the headers:
"\x1f\x8b" => true, // Gzip marker.
"\x78\x01" => true, // Zlib marker - level 1.
"\x78\x5e" => true, // Zlib marker - level 2 to 5.
"\x78\x9c" => true, // Zlib marker - level 6.
"\x78\xda" => true, // Zlib marker - level 7 to 9.
I don't remember how exactly I generated the output, it was on a Linux machine a very long time ago but probably with plain old gzip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also make sure you do echo -n
I think that's the difference in your compression test, @schlessera - the newline character. This means more data and a different CRC (last bytes).
echo -n 'foobar' | gzip -c | hexdump -v -e '"\\x" 1/1 "%02x"' ; echo
\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x03\x4b\xcb\xcf\x4f\x4a\x2c\x02\x00\x95\x1f\xf6\x9e\x06\x00\x00\x00
Which matches the cases :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @soulseekah for pitching in! That's helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, @soulseekah !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrfnl The above command should be added as a comment to the tests so that we know how to generate new cases in the future.
There are a number of test classes which all directly relate to the `WpOrg\Requests\Requests` class. Having those test files/classes all in the `tests` root directory does not make it any clearer what these tests are actually testing. So, I'm proposing that if a class warrants multiple test classes, we create a directory named after the class and place the files in that directory. Also, the tests in this file are not about _encoded_ data, but about _compressed_ data, so renaming the test class to be more descriptive.
Move the data providers (and associated helper methods) down to below the tests they apply to.
... to better reflect the data which is expected to be passed to the tests.
* Rename the methods to use `data` as the prefix. * Remove the `static` keyword as data providers do not need to be `static` (and haven't needed to be for a long time). * Add keys to the data arrays to turn the data sets into named data providers. * Add keys to the information in each data set to make the data providers easier to understand. * Remove the "helper" methods to "create" the data providers. They are unnecessary and obfuscate what data was provided to the tests. * Use all three data providers for each test instead of the helper method.
... to ensure that non-compressed data is returned unchanged.
... in `Requests::decompress()` and `Requests::compatible_gzinflate()`. Bow out earlier for non-compressed strings containing only whitespace. Includes extra test case and fixes the failing test for `Requests::compatible_gzinflate()` as introduced in the previous commit.
Includes adding `@covers` tags.
The `@since` tag should reference the version in which the method was introduced in Requests, not in WordPress.
e40562f
to
954efda
Compare
5809709
to
135d812
Compare
👉🏻 Note: the methods covered via this test class need a lot more tests, in particular the
Requests::compatible_gzinflate()
method, which includes code paths currently completely untested and handling of ZIP file format, which when called fromRequests::decompress()
(which is the only way it is called from within the Requests library itself), can never be reached as the "magic markers" used by ZIP are not included in theRequests::$magic_compression_headers
property.A future iteration should do a more in-depth review of these two method to determine what should be handled by each method and add additional tests. Still, this first iteration is already an improvement.
EncodingTest: move to subdirectory and rename
There are a number of test classes which all directly relate to the
WpOrg\Requests\Requests
class.Having those test files/classes all in the
tests
root directory does not make it any clearer what these tests are actually testing. So, I'm proposing that if a class warrants multiple test classes, we create a directory named after the class and place the files in that directory.Also, the tests in this file are not about encoded data, but about compressed data, so renaming the test class to be more descriptive.
DecompressionTest: reorder methods
Move the data providers (and associated helper methods) down to below the tests they apply to.
DecompressionTest: rename test method parameters
... to better reflect the data which is expected to be passed to the tests.
DecompressionTest: refactor data providers
data
as the prefix.static
keyword as data providers do not need to bestatic
(and haven't needed to be for a long time).DecompressionTest: add extra test cases
... to ensure that non-compressed data is returned unchanged.
Requests: improve handling of non-compressed semi-empty strings
... in
Requests::decompress()
andRequests::compatible_gzinflate()
.Bow out earlier for non-compressed strings containing only whitespace.
Includes extra test case and fixes the failing test for
Requests::compatible_gzinflate()
as introduced in the previous commit.DecompressionTest: improve documentation
Includes adding
@covers
tags.Requests::compatible_gzinflate: fix
@since
tagThe
@since
tag should reference the version in which the method was introduced in Requests, not in WordPress.Related to #497