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
Branch
factorize-storage-tests
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4058
Build 5334: tox-on-jenkinsJenkins
Build 5333: arc lint + arc unit

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
340

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
340

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.