Page MenuHomeSoftware Heritage

loader.tar: Add some tests and remove dead code
ClosedPublic

Authored by ardumont on Dec 7 2018, 8:04 PM.

Details

Summary

loader.tar.utils: Add tests to random_block function
swh.loader.tar.utils: Remove duplicated grouper definition

Related T1411
Depends on D793

Test Plan

tox

The tests won't build up until the diff is landed and a
new tag is pushed though.

Diff Detail

Repository
rDLDTAR Tarball Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Dec 7 2018, 8:04 PM

Are you sure these tests pass when the other Diff is merged?

Because the blocks in actual_data are shuffled, self.assertIn(d, expected_data) has a 1/block_size of raising an AssertionError on each loop iteration.

swh/loader/tar/tests/test_utils.py
14–26 ↗(On Diff #2518)

This method looks like assertCountEqual, with less checks on duplicated values (eg. assert_ok([1, 2, 2, 3], [1, 2, 3, 3]) succeeds).

Also, I don't understand why you shuffle expected_data.

swh/loader/tar/utils.py
32 ↗(On Diff #2518)

Extra point for un-shadowing that variable :)

ardumont updated this revision to Diff 2520.Dec 8 2018, 11:17 AM
  • utils: Simplify random_block according to latest api change
ardumont marked an inline comment as done.Dec 8 2018, 11:25 AM

Are you sure these tests pass when the other Diff is merged?

I temporarily copied the new implementation (each time) to make sure tests are fine.
So yes.

Because the blocks in actual_data are shuffled, self.assertIn(d, expected_data) has a 1/block_size of raising an AssertionError on each loop iteration.

I did not get it yet.
I do not expect errors as you mentioned.

In the end random_blocks yields all values as if it's one iterable. If you call list on it, it will return a list of elements (not a list of list of elements).
It's called random blocks because it randomizes the iterable (most likely a generator so no length) per block of a given size.

Not sure if i'm relevant here (like i said i did not get the remark yet ;)

swh/loader/tar/tests/test_utils.py
14–26 ↗(On Diff #2518)

Also, I don't understand why you shuffle expected_data.

Just because i don't want to check the order.
Not sure that's interesting to do so.

ardumont updated this revision to Diff 2521.Dec 8 2018, 11:27 AM

Fix docstring typo

vlorentz accepted this revision.Dec 8 2018, 11:43 AM

In the end random_blocks yields all values as if it's one iterable. If you call list on it, it will return a list of elements (not a list of list of elements).

Right!

This revision is now accepted and ready to land.Dec 8 2018, 11:43 AM
ardumont updated this revision to Diff 2526.Dec 8 2018, 6:05 PM

Trying to trigger the build again (new tag has been published)

This revision was automatically updated to reflect the committed changes.