Page MenuHomeSoftware Heritage

Factorize tests of the indexer storages.
ClosedPublic

Authored by vlorentz on Feb 7 2019, 2:05 PM.

Diff Detail

Repository
rDCIDX Object indexer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Feb 7 2019, 2:05 PM
vlorentz updated this revision to Diff 3432.Feb 7 2019, 2:13 PM
  • Generalize test_revision_metadata_add_duplicate_twice.

I'm not sure we gain here. It obfuscates the tests.

well, we do avoid duplication in test data but then the test data is sparse.
In the setup of the tests, we have a partial view of the overall expected results.

futhermore, it seems we lose some tests (e.g. ctags) for example.

I'm mitigated over this one. Another point of view would be nice
@douardda @anlambert any thoughts?

swh/indexer/tests/storage/test_storage.py
343

It seems we lose coverage here.
Do not migrate those tests to the new way and avoid losing coverage.

I'm not sure we gain here. It obfuscates the tests.

On the contrary, I find that factorizing them makes them easier to read, as I don't have to read five times almost-the-same-but-not-really code

well, we do avoid duplication in test data but then the test data is sparse.

We also avoid duplication of the test case (which we copy-pasted several times), sometimes with mistakes; that's why this factoring caught a bug.

futhermore, it seems we lose some tests (e.g. ctags) for example.

No we don't. I preserved tests that I could not factorize easily.

ardumont added inline comments.Feb 7 2019, 2:34 PM
swh/indexer/tests/storage/test_storage.py
343

vlorentz: No we don't. I preserved tests that I could not factorize easily.

cool

vlorentz marked 2 inline comments as done.Feb 7 2019, 2:36 PM

good points.

On the contrary, I find that factorizing them makes them easier to read, as I don't have to read five times almost-the-same-but-not-really code

Yes and no

Yes, the gist of it is.

No, because the way it's plugged together is not so clear (@rename decorator, tests generation)...
This is no longer simple tests (one might ask where are the tests for the tests plumbing? ;)

i won't block this though. Let's see where this goes.

vlorentz updated this revision to Diff 3434.Feb 7 2019, 2:59 PM
  • Rewrite as a parent of D1079
vlorentz updated this revision to Diff 3438.Feb 7 2019, 3:10 PM
  • Fix Diff reordering.
ardumont edited the summary of this revision. (Show Details)Feb 7 2019, 3:17 PM
vlorentz updated this revision to Diff 3443.Feb 7 2019, 3:22 PM
  • rebase
ardumont accepted this revision.Feb 7 2019, 3:24 PM

i won't block this though. Let's see where this goes.

here it goes.

This revision is now accepted and ready to land.Feb 7 2019, 3:24 PM
vlorentz updated this revision to Diff 3452.Feb 7 2019, 3:28 PM
  • rebase
This revision was automatically updated to reflect the committed changes.