Page MenuHomeSoftware Heritage

Add tests for swh.core.db.
ClosedPublic

Authored by vlorentz on Feb 11 2019, 3:43 PM.

Details

Summary

This increases coverage of the swh.core repository by:

  • initializing db tests
  • introducing hypothesis
  • refactoring a bit the connection initialization of the basedb module

Diff Detail

Repository
rDCORE Foundations and core functionalities
Branch
test-db
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4140
Build 5454: tox-on-jenkinsJenkins
Build 5453: arc lint + arc unit

Event Timeline

vlorentz created this revision.Feb 11 2019, 3:43 PM
ardumont added inline comments.
requirements-test.txt
4

(>= 3.11.0)

Related P356

vlorentz marked an inline comment as done.Feb 11 2019, 3:55 PM
vlorentz added inline comments.
requirements-test.txt
4

Even without regexps?

ardumont added inline comments.Feb 11 2019, 4:01 PM
requirements-test.txt
4

well, it works!

I added it to the scheduler (matching the one we already have on indexer) and nothing complained.

[1] is proof as it built (we can see debian tags and everything)

[1] https://forge.softwareheritage.org/rDSCH3488c265f4e3f70c8591dd1c7064388b312e9144

4

no parenthesis though, don't know why i put that there.

vlorentz updated this revision to Diff 3505.Feb 11 2019, 4:04 PM
  • bump hypothesis version
anlambert accepted this revision.Feb 11 2019, 4:12 PM
anlambert added a subscriber: anlambert.

Looks good to me!

swh/core/tests/test_db.py
61

Just a nitpick, I would put the strategy implementation into a dedicated function and named it according to your desired test inputs instead.
I think tests are more readable this way.

This revision is now accepted and ready to land.Feb 11 2019, 4:12 PM
ardumont added inline comments.Feb 11 2019, 4:13 PM
swh/core/db/__init__.py
112

I'm unsure of that factorisation (because i'm not so sure of the workflow around those calls: from_pool, connect).
What i'm sure though is that it could be cause of regression.
I'd feel safer if we added tests around those cases.

Reading swh.storage.{storage,db} and scheduler's, the initialization sounds fine with this change but still...

ardumont edited the summary of this revision. (Show Details)Feb 11 2019, 4:14 PM

Not sure about the initialization refactoring and its impacts on the other modules.

Other than that, the rest sounds quite fine, thanks.

vlorentz marked an inline comment as done.Feb 11 2019, 4:29 PM
vlorentz added inline comments.
swh/core/db/__init__.py
112

That will only be an issue when a connection object is created directly, instead of using one of these classmethods. grep "psycopg2\.conn" ../*/swh/ -r shows only two results outside swh.core: the vault (before D1108) and the swh.journal.checker. Neither use BaseDb.

I'd feel safer if we added tests around those cases.

Which ones?

Which ones?

the @classmethod you changed from_pool and connect?

vlorentz updated this revision to Diff 3507.Feb 11 2019, 4:31 PM
  • Rebase
  • Move the hypothesis given outside the class.
vlorentz updated this revision to Diff 3508.Feb 11 2019, 4:47 PM
  • Add test for BaseDb.connect.
This revision was automatically updated to reflect the committed changes.