Page MenuHomeSoftware Heritage

Add test for content_get_range.
Needs ReviewPublic

Authored by vlorentz on Wed, Jun 5, 1:30 PM.

Details

Reviewers
None
Group Reviewers
Reviewers

Diff Detail

Repository
rDSTO Storage manager
Branch
test-content_get_range
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6208
Build 8569: tox-on-jenkinsJenkins
Build 8568: arc lint + arc unit

Event Timeline

vlorentz created this revision.Wed, Jun 5, 1:30 PM
douardda added inline comments.
swh/storage/tests/test_storage.py
3199

probably a stupid question but why do we expect 3 here (instead of 4)?

vlorentz added inline comments.Wed, Jun 12, 12:27 PM
swh/storage/tests/test_storage.py
3199

Because we're getting contents in range get_sha1s[1] to get_sha1s[-1] inclusive, so that's all the contents (4) minus the first one.

douardda added inline comments.Thu, Jun 13, 2:14 PM
swh/storage/tests/test_storage.py
3199

oh yes indeed.

Wouldn't it be useful (?) to raise the max_size and check the content_get_range against all situations wrt range limits: the beginning, middle and end of the range (like range(0, 2), range(n, n+2) and range(-3, -1) )?

vlorentz added inline comments.Thu, Jun 13, 2:17 PM
swh/storage/tests/test_storage.py
3199

That's what the three tests are for (test_generate_content_get_range_start, test_generate_content_get_range_end, test_origin_get_range). I did not want to do a single test for all of them, because it means writing the same logic in the test as in the real code, so testing makes less sense

douardda added inline comments.Thu, Jun 13, 2:30 PM
swh/storage/tests/test_storage.py
3199

Oh you mean I must read the whole diff when I review it? what a bama.

So now, why does the range_start test uses sha1[1] as start value?

vlorentz added inline comments.Fri, Jun 14, 10:56 AM
swh/storage/tests/test_storage.py
3199

range_start tests that nothing before the start is returned, range_end tests that nothing after the end is returned.

3199

Eh, you're right, I could merge these two tests

vlorentz updated this revision to Diff 5234.Fri, Jun 14, 12:21 PM

Merge both tests, and use more input parameters to test all cases at once.