Page MenuHomeSoftware Heritage

tests: Separate lister instantiations
ClosedPublic

Authored by ardumont on Sep 2 2020, 10:00 AM.

Details

Summary

Prior to this commit, all listers were instantiated at the same time even if
only one was needed. This commit separates those instantiations.

The only drawback to this is the db model initialization which now happens at
each lister instantiation. This can be dealt with if needed at another time
though.

Depends on D3859

Test Plan

tox

Diff Detail

Repository
rDLS Listers
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14868
Build 22907: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 22906: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D3860 (id=13634)

Could not rebase; Attempt merge onto e99d3464e4...

Updating e99d346..94c40a1
Fast-forward
 MANIFEST.in                                        |  1 +
 conftest.py                                        | 20 +---------
 swh/lister/bitbucket/tests/conftest.py             |  1 -
 swh/lister/bitbucket/tests/test_lister.py          | 12 ++++--
 swh/lister/cgit/tests/conftest.py                  |  1 -
 swh/lister/cgit/tests/test_lister.py               | 28 ++++++++------
 swh/lister/cran/tests/conftest.py                  | 15 ++++----
 swh/lister/debian/tests/conftest.py                | 18 +++++----
 swh/lister/gitea/tests/conftest.py                 |  6 ---
 swh/lister/gitea/tests/test_lister.py              | 11 +++++-
 swh/lister/github/tests/conftest.py                |  1 -
 swh/lister/github/tests/test_lister.py             | 14 ++++---
 swh/lister/gitlab/tests/conftest.py                |  6 ---
 swh/lister/gitlab/tests/test_lister.py             | 16 +++++---
 swh/lister/gnu/tests/conftest.py                   |  6 ---
 swh/lister/gnu/tests/test_lister.py                | 16 +++++---
 swh/lister/launchpad/tests/conftest.py             |  1 -
 swh/lister/npm/tests/conftest.py                   | 17 +++++----
 swh/lister/packagist/tests/conftest.py             | 15 ++++----
 swh/lister/phabricator/tests/conftest.py           | 17 +++++----
 swh/lister/pypi/tests/conftest.py                  | 17 +++++----
 .../{core/tests/conftest.py => pytest_plugin.py}   | 44 +++++++++++++---------
 swh/lister/tests/test_cli.py                       |  7 ++--
 23 files changed, 153 insertions(+), 137 deletions(-)
 delete mode 100644 swh/lister/bitbucket/tests/conftest.py
 delete mode 100644 swh/lister/cgit/tests/conftest.py
 delete mode 100644 swh/lister/gitea/tests/conftest.py
 delete mode 100644 swh/lister/github/tests/conftest.py
 delete mode 100644 swh/lister/gitlab/tests/conftest.py
 delete mode 100644 swh/lister/gnu/tests/conftest.py
 rename swh/lister/{core/tests/conftest.py => pytest_plugin.py} (51%)
Changes applied before test
commit 94c40a1474f0f5e48b29d1f5c16cb4a9aa6897a2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 2 09:47:12 2020 +0200

    tests: Separate lister instantiations
    
    Prior to this commit, all listers were instantiated at the same time even if
    only one was needed. This commit separates those instantiations.
    
    The only drawback to this is the db model initialization which now happens at
    each lister instantiation. This can be dealt with if needed at another time
    though.

commit cdbccd56fce040ac11357e3e974d44e041d97373
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 1 17:55:23 2020 +0200

    pytest_plugin: Instantiate only lister with no particular setup
    
    This should fix the remaining blocking problems in the jenkins build failure
    report [1]
    
    [1] https://jenkins.softwareheritage.org/view/Debian%20packages/job/debian/job/packages/job/DLS/job/gbp-buildpackage/78/consoleFull

commit 52898aa4bec08500f16707d17e2c45eb4ef3a2e9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 1 17:43:01 2020 +0200

    pytest: Define plugin and declare it in the root conftest
    
    Then drop all unneeded and indirect imports

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

olasd added a subscriber: olasd.

Ah, that looks like a much more sensible way to do this!

I'd have a few nitpicks (maybe calling the fixture lister_under_test instead of lister_name_to_test, or replacing the lister_xyz fixtures sprinkled throughout the conftest.pys with a common initialized_lister fixture, that would default to passing through the swh_lister fixture), but as this code is mostly on the way out (knock on wood), I think it's fine as is (and definitely an improvement over the status quo).

This revision is now accepted and ready to land.Sep 2 2020, 10:14 AM

lister_under_test instead of lister_name_to_test

yes, i'll rename it like this, it sounds better.

Rename fixture to lister_under_test

Build is green

Patch application report for D3860 (id=13640)

Rebasing onto 92422dcf75...

Current branch diff-target is up to date.
Changes applied before test
commit 5a5b7ef70bf997695d2388f67fc63ffe9be1394e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Sep 2 09:47:12 2020 +0200

    tests: Separate lister instantiations
    
    Prior to this commit, all listers were instantiated at the same time even if
    only one was needed. This commit separates those instantiations.
    
    The only drawback to this is the db model initialization which now happens at
    each lister instantiation. This can be dealt with if needed at another time
    though.

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

This revision was automatically updated to reflect the committed changes.

replacing the lister_xyz fixtures sprinkled throughout the conftest.pys
with a common initialized_lister fixture, that would default to passing
through the swh_lister fixture), but as this code is mostly on the way out
(knock on wood), I think it's fine as is (and definitely an improvement over
the status quo).

That sounded reasonable enough so D3861 ¯\_(ツ)_/¯