Details

Reviewers

vlorentz
ardumont

Group Reviewers

Reviewers

Commits

rDSTOCfb70f881cb37: tests: refactor main storage tests
rDSTOC9f122027e8cd: conftest: use an in memory obj storage
rDSTO9f122027e8cd: conftest: use an in memory obj storage
rDSTOfb70f881cb37: tests: refactor main storage tests

Summary

use pytest instead of unittest.TestCase plumbing
extract data from the TestStorageData into a data storage_data module; this module also provide a simple helper StorageData class that mimics the original class (access by attributes),
implement a series of pytest fixtures for these storage specific tests,
get rid of most hypothesis-based tests.

Diff Detail

Repository

rDSTO Storage manager

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

ardumont added a subscriber: ardumont.Oct 8 2019, 5:29 PM

ardumont added inline comments.

swh/storage/tests/conftest.py
41	'memory' (we fixed it together ;) also olasd migrated the existing entries in storage recently.

mark test_in_memory and test_api_client as xfail

until D2095 and D2119

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/695/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/695/console

Harbormaster failed remote builds in B8339: Diff 7137!Oct 11 2019, 3:19 PM

rebase + fix tests in tox (update mypy.ini and requirements-test.txt)

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/696/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/696/console

Harbormaster failed remote builds in B8342: Diff 7140!Oct 11 2019, 3:48 PM

also xfail test_snapshot (until it has been ported to pytest/pytest_postgresql)

in D2117

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/697/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/697/console

Harbormaster failed remote builds in B8343: Diff 7141!Oct 11 2019, 3:59 PM

xfail does not mean xerror...

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tox/698/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tox/698/console

Harbormaster failed remote builds in B8344: Diff 7142!Oct 11 2019, 4:10 PM

also fix test_db to not depends on running tests via pifpaf

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/705/ for more details.

Harbormaster completed remote builds in B8363: Diff 7153.Oct 11 2019, 4:25 PM

douardda added a child revision: D2092: test_storage: move db specific methods from main TestStorage to TestPgStorage.Oct 11 2019, 4:31 PM

douardda removed a child revision: D2091: test_storage: kill StorageTestDbFixture class.

use memory cls for the journal_writer instead of deprecated inmemory

I think the xfails should have a comment for future readers stumbling upon this commit.

swh/storage/tests/conftest.py
49–60	not done
59	not done
91–95	not done
tox.ini
28–35	Why?

This revision now requires changes to proceed.Oct 11 2019, 4:45 PM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/706/ for more details.

Harbormaster completed remote builds in B8366: Diff 7156.Oct 11 2019, 4:46 PM

douardda added inline comments.Oct 11 2019, 4:54 PM

swh/storage/tests/conftest.py
91–95	well, I would have expected a comment about the fact there is no comment in this code stating that this function is stolen from pytest-postgresql ;-), and explaining why it is needed.
tox.ini
28–35	this chunk could be moved in D2095 I guess. I removed it since I gradually drop support for ENABLE_ORIGIN_IDS in tests.

Reactivate [testenv:py3-no-origin-ids] in tox

vlorentz added inline comments.Oct 11 2019, 5:07 PM

tox.ini
28–35	`py3-no-origin-ids` runs tests for the new behavior. Now tox only runs tests for the old behavior.

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/707/ for more details.

Harbormaster completed remote builds in B8369: Diff 7159.Oct 11 2019, 5:09 PM

add some comment in conftest to explain a bit the postgres stuff

and add sha1_git as unicity key for the hypothesis based gen_contents
helper function.

douardda marked 4 inline comments as done.Oct 11 2019, 5:26 PM

douardda added inline comments.

swh/storage/tests/conftest.py
49–60	I do prefer `gen_xxx` since it's a function that generates these objects. `xxx_lists` seems less clear to me.

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/708/ for more details.

Harbormaster completed remote builds in B8370: Diff 7160.Oct 11 2019, 5:27 PM

vlorentz requested changes to this revision.Oct 11 2019, 5:32 PM

vlorentz added inline comments.

swh/storage/tests/conftest.py
49–60	but it doesn't follow Hypothesis' naming convention.
63–76	If we're going with the `gen_` prefix, then these functions should have it too. And `swh_origins` is way too different from `swh_contents` to have such a close name.
73	why 20? using these many origins even for tests that won't use it wil make Hypothesis explore much more space than needed.

This revision now requires changes to proceed.Oct 11 2019, 5:32 PM

douardda marked an inline comment as done.Oct 11 2019, 5:57 PM

douardda added inline comments.

swh/storage/tests/conftest.py
73	Have you noticed I do not use hypothesis for exploring anything here? I'm using it as an easy way of generating (fixed-size) datasets. But, it would make sense to use the gen_origins() in this fixture, instead of reimplementing it.

douardda added inline comments.Oct 11 2019, 5:59 PM

swh/storage/tests/conftest.py
63–76	I agree with you, the names for swh_origins and swh_contents are confusing here.
73	20: because I do not ask hypothesis to explore anything, I just generate ONE list of 20 (by default) origin/content objects.

ardumont added inline comments.Oct 11 2019, 6:38 PM

swh/storage/tests/conftest.py
17	Don't we want a fixture `sqldir` similar to the datadir we introduced in swh-core for the sql files?
37	Isn't a memory objstorage enough?
73	But, it would make sense to use the gen_origins() in this fixture, instead of reimplementing it. Yes, i don't get why you did not do it since it's the same code ;)

ardumont added inline comments.Oct 11 2019, 6:43 PM

swh/storage/tests/conftest.py
81	I don't even know what's the DBJanitor you are talking about ;)

ardumont added inline comments.Oct 11 2019, 6:46 PM

swh/storage/tests/data/storage.yml
9	? If we use a memory objstorage, we can skip that (i guess there is some stuff replacing this somewhere ;)
13	memory but that might be fixed in other commits...

Looks good to me.

Got a couple of questions/remarks but nothing too blocking.

ardumont accepted this revision.Oct 11 2019, 6:46 PM

fix definition and usage of swh_contents and swh_origins fixtures

also add a revision that use an in memory obj storage in swh_storage fixture

douardda added a child revision: D2091: test_storage: kill StorageTestDbFixture class.Oct 14 2019, 10:56 AM

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/709/ for more details.

Harbormaster completed remote builds in B8388: Diff 7174.Oct 14 2019, 10:57 AM

ardumont accepted this revision.Oct 14 2019, 11:06 AM

revert usage for self._test_origin_ids and use_url to fix py3-no-origin-ids

also replace boolean hypothesis strategy by pytest.mark.parametrize, so we do
not need to reset the storage in these tests (since the test is truly
executed twice).

Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/718/ for more details.

Harbormaster completed remote builds in B8398: Diff 7184.Oct 14 2019, 12:15 PM

In D2090#49800, @douardda wrote:

also replace boolean hypothesis strategy by pytest.mark.parametrize, so we do
not need to reset the storage in these tests (since the test is truly
executed twice).

Nice!

douardda removed a child revision: D2091: test_storage: kill StorageTestDbFixture class.Oct 14 2019, 12:24 PM

douardda edited the summary of this revision. (Show Details)Oct 14 2019, 1:04 PM

douardda edited the test plan for this revision. (Show Details)

ardumont accepted this revision.Oct 14 2019, 1:13 PM

vlorentz accepted this revision.Oct 14 2019, 1:19 PM

This revision is now accepted and ready to land.Oct 14 2019, 1:19 PM