Page MenuHomeSoftware Heritage

tests: migrate indexer storage tests to pytest
ClosedPublic

Authored by douardda on Nov 6 2019, 5:49 PM.

Diff Detail

Repository
rDCIDX Metadata indexer
Branch
tests
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 8886
Build 12971: tox-on-jenkinsJenkins
Build 12970: arc lint + arc unit

Event Timeline

ardumont retitled this revision from tests: migrate storage tests to pytest to tests: migrate indexer storage tests to pytest.Nov 7 2019, 10:00 AM
ardumont added inline comments.
swh/indexer/tests/storage/conftest.py
113

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
75

bad name (_w_)

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

Why an uuid instead of os.random?

106

same

vlorentz requested changes to this revision.Nov 7 2019, 1:43 PM
This revision now requires changes to proceed.Nov 7 2019, 1:43 PM

rebase + keep the test_storage.py file for now

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

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

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

I do not agree, but meh

113

heuuu this is the conftest.py file...

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
89

Is there a problem with using uuid there?

no, it just doesn't make sense

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
89

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.

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

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.

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

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

update/add headers in test files

This revision is now accepted and ready to land.Nov 8 2019, 3:45 PM