Page MenuHomeSoftware Heritage

tests: migrate indexer storage tests to pytest
ClosedPublic

Authored by douardda on Wed, Nov 6, 5:49 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

douardda created this revision.Wed, Nov 6, 5:49 PM
ardumont retitled this revision from tests: migrate storage tests to pytest to tests: migrate indexer storage tests to pytest.Thu, Nov 7, 10:00 AM
ardumont added inline comments.
swh/indexer/tests/storage/conftest.py
114

I'm just curious: why don't you define the fixture definition in conftest.py?

swh/indexer/tests/storage/test_in_memory.py
1

You are missing headers.

swh/indexer/tests/storage/test_indexer_storage.py
87 ↗(On Diff #7679)

expect

Why not inheritance instead of so many assigments? (Also, non-methods with a "self" argument...)

And I would prefer explicitly skipping/removing tests in each class than whitelisting which ones run

swh/indexer/tests/storage/conftest.py
76

bad name (_w_)

swh/indexer/tests/storage/generate_data_test.py
90

Why an uuid instead of os.random?

107

same

vlorentz requested changes to this revision.Thu, Nov 7, 1:43 PM
This revision now requires changes to proceed.Thu, Nov 7, 1:43 PM
douardda updated this revision to Diff 7702.Thu, Nov 7, 4:20 PM

rebase + keep the test_storage.py file for now

douardda added inline comments.Thu, Nov 7, 4:43 PM
swh/indexer/tests/storage/generate_data_test.py
90

because the uuid hypothesis generator is used in gen_content_mimetypes() I guess, so I stuck to it without really thinking about it. And it's a simple way of generating a small random string/bytes. Is there a problem with using uuid there?

swh/indexer/tests/storage/test_in_memory.py
1

not really here (but I could have added them obviously) but I do in test_storage.py for sure

douardda added inline comments.Thu, Nov 7, 4:46 PM
swh/indexer/tests/storage/conftest.py
76

I do not agree, but meh

114

heuuu this is the conftest.py file...

douardda updated this revision to Diff 7717.Thu, Nov 7, 5:33 PM

rebase + better ci message + s/_w_/_with_/g

You missed one of my comments:

Why not inheritance instead of so many assigments? (Also, non-methods with a "self" argument...)
And I would prefer explicitly skipping/removing tests in each class than whitelisting which ones run

swh/indexer/tests/storage/generate_data_test.py
90

Is there a problem with using uuid there?

no, it just doesn't make sense

douardda updated this revision to Diff 7719.Fri, Nov 8, 10:14 AM

Refactor the tests to prevent these ugly injected functions

doing so, also break the TestIndexerStorageOthers class in pieces
(one for each covered endpoint type).

swh/indexer/tests/storage/generate_data_test.py
90

why so? uuid is a perfectly valid way of generating a small random piece of bytes. Also, it's not clear if this really is a blocker for you or not.

ardumont added inline comments.Fri, Nov 8, 1:09 PM
swh/indexer/tests/storage/conftest.py
114

ah, lol, wrong file, i meant for the test_api_client for example.
But that must be for sandboxing where it's needed, i guess.

douardda updated this revision to Diff 7734.Fri, Nov 8, 1:58 PM

rebase (hopefully)

douardda added inline comments.Fri, Nov 8, 2:11 PM
swh/indexer/tests/storage/conftest.py
114

in these files, it is indeed a local overload of the given fixture.

douardda updated this revision to Diff 7737.Fri, Nov 8, 2:14 PM

update/add headers in test files

douardda marked an inline comment as done.Fri, Nov 8, 2:15 PM
douardda edited the summary of this revision. (Show Details)Fri, Nov 8, 3:21 PM
vlorentz accepted this revision.Fri, Nov 8, 3:45 PM
This revision is now accepted and ready to land.Fri, Nov 8, 3:45 PM