Paths

Table of Contentst

Differential D3428

Migrate loader tests to use pytest
ClosedPublic
Actions

Authored by ardumont on Jul 6 2020, 1:49 PM.

Details

Reviewers

None

Group Reviewers

Reviewers

Maniphest Tasks

T2482: Migrate git loader tests code to pytest

Commits

rDLDGf0e48ff7810a: Migrate loader tests to use pytest

Summary

This simplifies the base classes indirection introduced to make the tests run.

It keeps the unittest scaffolding to allow declaring tests once and reuse
amongst the different loader instances (GitLoader, GitLoaderFromDisk,
GitLoaderFromArchive).

This uses the pg-storage and no longer the in-memory storage.

The code coverage increased from 82% to 85%.

This duplicates a bit helper function from the loader-core (equivalent code
exists in loader-svn). I'll attend to this to remove that duplication soon (the
diff could land without ;)

Related to T2482

Test Plan

tox

Diff Detail

Repository

rDLDG Git loader

Branch

migrate-pytest

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 13364
Build 20427: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 20426: arc lint + arc unit

Event Timeline

ardumont created this revision.Jul 6 2020, 1:49 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptJul 6 2020, 1:49 PM

Build is green

Patch application report for D3428 (id=12142)

Rebasing onto 55ff84b1a6...

Current branch diff-target is up to date.

Changes applied before test

commit 07419a649ec0c02f98d2a0e8ee62c5b99dacf1d8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 6 12:41:57 2020 +0200

    Migrate loader tests to use pytest
    
    This simplifies the base classes indirection introduced to make the tests run.
    
    It keeps the unittest scaffolding to allow declaring tests once and reuse
    amongst the different loader instances (GitLoader, GitLoaderFromDisk,
    GitLoaderFromArchive).
    
    The code coverage increased from 82% to 85%.

See https://jenkins.softwareheritage.org/job/DLDG/job/tests-on-diff/27/ for more details.

Harbormaster completed remote builds in B13364: Diff 12142.Jul 6 2020, 1:50 PM

ardumont edited the summary of this revision. (Show Details)Jul 6 2020, 1:50 PM

ardumont added inline comments.

swh/loader/git/tests/__init__.py
23	I will definitely move that to the loader-core now. Also, note that the `uncompress_archive` option is no longer needed (i dropped its use).
swh/loader/git/tests/conftest.py
50	That should probably go up in the loader-core now.

ardumont added a task: T2482: Migrate git loader tests code to pytest.Jul 6 2020, 1:57 PM

ardumont edited the summary of this revision. (Show Details)

ardumont edited the summary of this revision. (Show Details)Jul 6 2020, 2:06 PM

ardumont added inline comments.Jul 6 2020, 3:32 PM

swh/loader/git/tests/__init__.py
23	D3431

That's pretty nice, thanks.

Could we make the check_snapshot function check object references recursively? I think checking the statistics output by the loader is already pretty good, but we've had, in the past, loaders generate buggy edges. Making a recursive check of all objects pointed at by the snapshot would alleviate these concerns completely (and for all loaders).

MANIFEST.in
5	No need for both `` and `.xz`.
requirements-test.txt
3	I don't see you instantiating a postgresql anywhere. I guess this should read `swh.storage[testing]`.
swh/loader/git/tests/conftest.py
14	I understand why this is going on, but instead of generalizing this (somewhat clunky) pattern I think the relevant fixtures should be properly documented and moved to a dedicated pytest plugin in the source modules. I've ended up doing it for swh.scheduler in D3430, it's not too bad. (No need to do it right now: you can keep this `import `, do the refactoring of swh.storage test fixtures in parallel, and drop the `import ` when that's done. On the release that moves the pytest fixtures, they'll get auto-imported instead of being pulled in by the `import *`, so the switch over is backwards compatible.)

Could we make the check_snapshot function check object references recursively?

Yes! Good idea.

I'll add this to my todo list on refactoring then [1] ;)

[1] T2483

I think checking the statistics output by the loader is already pretty good, but we've had, in the past, loaders generate buggy edges. Making a recursive check of all objects pointed at by the snapshot would alleviate these concerns completely (and for all loaders).

Indeed.

swh/loader/git/tests/conftest.py
14	right!

ardumont added inline comments.Jul 6 2020, 3:55 PM

requirements-test.txt
3	oh yeah indeed, i did not realize i could do it that way. thanks.

ardumont added inline comments.Jul 6 2020, 3:58 PM

swh/loader/git/tests/conftest.py
14	T2484 (I took my old approach back from the origin-visit immutable. This time, create dedicated subtask with a dedicated perimeter instead of letting a task opened too long with an almost ever growing perimeter ;)

Adapt according to review

Build has FAILED

Patch application report for D3428 (id=12154)

Rebasing onto 55ff84b1a6...

Current branch diff-target is up to date.

Changes applied before test

commit f78bb8acce06baee86a974e68d9cac6ccbe0f077
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 6 12:41:57 2020 +0200

    Migrate loader tests to use pytest
    
    This simplifies the base classes indirection introduced to make the tests run.
    
    It keeps the unittest scaffolding to allow declaring tests once and reuse
    amongst the different loader instances (GitLoader, GitLoaderFromDisk,
    GitLoaderFromArchive).
    
    The code coverage increased from 82% to 85%.
    
    Related to T2482

Link to build: https://jenkins.softwareheritage.org/job/DLDG/job/tests-on-diff/28/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDG/job/tests-on-diff/28/console

Harbormaster failed remote builds in B13375: Diff 12154!Jul 6 2020, 4:02 PM

In D3428#84315, @swh-public-ci wrote:

Build has FAILED

I guess swh.storage[testing] also needs to be added explicitly to tox.ini dependencies. Or pytest-postgresql. *grmbl*

Add swh.storage[testing] to tox.ini, tox you happy?

Build is green

Patch application report for D3428 (id=12155)

Rebasing onto 55ff84b1a6...

Current branch diff-target is up to date.

Changes applied before test

commit f0e48ff7810a0bb3daa5c0e4ded92601e2547540
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 6 12:41:57 2020 +0200

    Migrate loader tests to use pytest
    
    This simplifies the base classes indirection introduced to make the tests run.
    
    It keeps the unittest scaffolding to allow declaring tests once and reuse
    amongst the different loader instances (GitLoader, GitLoaderFromDisk,
    GitLoaderFromArchive).
    
    The code coverage increased from 82% to 85%.
    
    Related to T2482

See https://jenkins.softwareheritage.org/job/DLDG/job/tests-on-diff/29/ for more details.

Harbormaster completed remote builds in B13376: Diff 12155.Jul 6 2020, 4:20 PM

This revision was not accepted when it landed; it landed in state Needs Review.Jul 6 2020, 4:30 PM

Closed by commit rDLDGf0e48ff7810a: Migrate loader tests to use pytest (authored by ardumont). · Explain Why

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDLDGf0e48ff7810a: Migrate loader tests to use pytest.

ardumont mentioned this in D3438: tests: Reuse loader-core test utility function.Jul 6 2020, 6:10 PM

ardumont mentioned this in D3440: loader.hg: Initialize and refactor deprecated tests to use pytest fixture.Jul 7 2020, 10:06 AM

ardumont mentioned this in D3435: Add mercurial.from_disk.HgLoaderFromDisk.Jul 7 2020, 10:55 AM