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

Failing Travis tests #55

Closed
oogali opened this issue Jul 28, 2016 · 13 comments
Closed

Failing Travis tests #55

oogali opened this issue Jul 28, 2016 · 13 comments

Comments

@oogali
Copy link
Contributor

oogali commented Jul 28, 2016

@esc So, it looks we have some test failures in Travis, but they're not what I expected?

The errors seem to fall into two categories.

  1. It looks like the expected compression ratio has been cut in half.

  2. The newer version of blosc introduces the zstd compression library, in which the tests don't expect that in the output.

The latter is easily fixable by updating the tests. The former... not so much.

@esc
Copy link
Member

esc commented Jul 28, 2016

Correct, initially I was very pedantic about maximum test coverage, maybe the ratio tests are too strict though and perhaps should be refactored to simply expect an output of a certain length, rather than a specific ratio. I will do it eventually, but don't mind being pre-empted. :) 👍

@esc
Copy link
Member

esc commented Jul 28, 2016

@esc
Copy link
Member

esc commented Jul 28, 2016

You can probably use regular expressions in the cram tests, I've done that already in a few places, feel free to use that as template.

@esc
Copy link
Member

esc commented Jul 28, 2016

The zstd this is a real bug in the tests, btw...

@oogali
Copy link
Contributor Author

oogali commented Jul 28, 2016

Would this be an appropriate change for the ratio tests?

diff --git a/test_cmdline/test_append.cram b/test_cmdline/test_append.cram
index 67cc677..1a7bf29 100644
--- a/test_cmdline/test_append.cram
+++ b/test_cmdline/test_append.cram
@@ -12,12 +12,14 @@ Create a test datafile.

 Test basic append:

+  $ ls -lah  data.dat
+  .* 1 .* .* \d{3}M .* .* .* data.dat (re)
   $ blpk compress data.dat
   $ ls -lah  data.dat.blp
-  .* 1 .* .* 13M .* .* .* data.dat.blp (re)
+  .* 1 .* .* \d{2}M .* .* .* data.dat.blp (re)
   $ blpk append data.dat.blp data.dat
   $ ls -lah  data.dat.blp
-  .* 1 .* .* 26M .* .* .* data.dat.blp (re)
+  .* 1 .* .* \d{2}M .* .* .* data.dat.blp (re)

 Cleanup.

@esc
Copy link
Member

esc commented Jul 28, 2016

Yeah, that would be fine.

@oogali
Copy link
Contributor Author

oogali commented Jul 30, 2016

I want to respect the pedantry, but I'm not quite sure on how to do that without making the tests a bit more complex.

If I use regular expressions, \d+ lets me match any digit... including zeroes. So if a 0B file is emitted, the tests will pass.

In order to guard against that, I only see two options:

  • Change all the digit matching from \d+ to something along the lines of [1-9]\d*
  • Pepper the tests with if statements, which then negates using cram for testing...

Halp?

@esc
Copy link
Member

esc commented Aug 3, 2016

How about [1-9]\d* ? Does that work? 0B files would be OK too, I guess... Putting if statements is a no-go.

@esc
Copy link
Member

esc commented Aug 3, 2016

BTW: I promise to make a release within 24 hours after this issues has been fixed. 😁

@oogali
Copy link
Contributor Author

oogali commented Aug 8, 2016

I have all of the tests fixed to use regular expressions to match digits (and match non-zero numbers where needed).

I had to then slice up some of the dictionaries to do a partial match (exclude compression ratio and sizes from the test), hope that's okay.

However, we seem to have a real test failure in test_headers.test_decode_blosc_header (see: https://travis-ci.org/oogali/bloscpack/builds/149805682). It looks like the flag change isn't picked up by the decode function.

@esc
Copy link
Member

esc commented Aug 8, 2016

I guess the way the internal flags are stored/used may have changed. cc @FrancescAlted

@esc
Copy link
Member

esc commented Aug 8, 2016

Actually, the problem lies elsewhere, my mistake. The no-shuffle flag is being picked up already (!). It seems like the problem lies with the test-data. This data is compressible when shuffled but--probably due to changes in blosc itself--are now suddenly incompressible when not shuffled. This is why the flag has the value two/second bit. You can try this for yourself with the following patch:

diff --git i/test/test_headers.py w/test/test_headers.py
index 211cf7e330..768175863d 100644
--- i/test/test_headers.py
+++ w/test/test_headers.py
@@ -140,6 +140,8 @@ def test_decode_blosc_header():
     header_slice = dict((k, header[k]) for k in expected.keys())
     nt.assert_equal(expected, header_slice)
     # deactivate shuffle
+
+    array_ = np.ones(16000, dtype=np.uint8)
     blosc_args.shuffle = False
     compressed = blosc.compress(array_, **blosc_args)
     header = decode_blosc_header(compressed)

This data is compressible for sure and the flags are neither 1 (shuffle) nor 2 (pure memcpy/incompressible) nor 3 (both).

@esc
Copy link
Member

esc commented Aug 22, 2016

closed by #57

@esc esc closed this as completed Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants