Page MenuHomeSoftware Heritage

Factorize tests of the indexer storages.
ClosedPublic

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

Diff Detail

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

Event Timeline

  • 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
342

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.

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

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

cool

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.

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
This revision was automatically updated to reflect the committed changes.