Page MenuHomeSoftware Heritage

package.loader: Retrieve latest snapshot out of the latest visit status
ClosedPublic

Authored by ardumont on Jun 17 2020, 4:24 PM.

Details

Summary

Preparatory work to remove origin visit unused fields

Related to T2310

Depends on D3321

Test Plan

tox

Diff Detail

Unit TestsFailed

TimeTest
418 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.package.archive.tests.test_archive::test_2_visits_with_new_artifact
swh_config = '/tmp/pytest-of-jenkins/pytest-0/test_2_visits_with_new_artifac0/loader.yml' requests_mock_datadir = <requests_mock.mocker.Mocker object at 0x7f44372e7c88>
326 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.package.archive.tests.test_archive::test_2_visits_without_change
swh_config = '/tmp/pytest-of-jenkins/pytest-0/test_2_visits_without_change0/loader.yml' requests_mock_datadir = <requests_mock.mocker.Mocker object at 0x7f44373005c0>
296 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.package.archive.tests.test_archive::test_2_visits_without_change_not_gnu
swh_config = '/tmp/pytest-of-jenkins/pytest-0/test_2_visits_without_change_n0/loader.yml' requests_mock_datadir = <requests_mock.mocker.Mocker object at 0x7f4437358278>
289 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.package.cran.tests.test_cran::test_cran_2_visits_same_origin
swh_config = '/tmp/pytest-of-jenkins/pytest-0/test_cran_2_visits_same_origin0/loader.yml' requests_mock_datadir = <requests_mock.mocker.Mocker object at 0x7f44372a30b8>
254 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.loader.package.nixguix.tests.test_nixguix::test_loader_incremental
swh_config = '/tmp/pytest-of-jenkins/pytest-0/test_loader_incremental0/loader.yml' requests_mock_datadir = <requests_mock.mocker.Mocker object at 0x7f4436849c50>
View Full Test Results (9 Failed · 130 Passed)

Event Timeline

Build is green

Patch application report for D3305 (id=11708)

Rebasing onto e7e27a16cd...

Current branch diff-target is up to date.
Changes applied before test
commit fcafbf94fe017d23dd00285703460a4b28c33d9a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 12:36:52 2020 +0200

    package.loader: Retrieve latest snapshot out of the latest visit status
    
    Related to T2310

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

  • Use latest storage
  • move assert_last_visit_ok to swh.loader.tests.conftest (so it can be imported for tests in other loader modules)

Tests should pass now

Build is green

Patch application report for D3305 (id=11752)

Rebasing onto e7e27a16cd...

Current branch diff-target is up to date.
Changes applied before test
commit 2ebf82ae01ce16bb929f4fe766bd35762474f907
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 12:36:52 2020 +0200

    package.loader: Retrieve latest snapshot out of the latest visit status
    
    Related to T2310

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

swh/loader/tests/conftest.py
66 ↗(On Diff #11752)

Pondering whether:

  • it's in the right location
  • to add tests on it (inclined to do so with mocks)

I intend to use it in other loader diffs [1] (which currently duplicates it)

[1] D3306 D3307 D3308

olasd added inline comments.
swh/loader/tests/conftest.py
66 ↗(On Diff #11752)

I'm not sure importing a function from conftest.py is the right move, but we seem to be doing that in a bunch of other places already, so shrug.

I guess the thing that makes me most uncomfortable is the prospect of having other modules import stuff from this module's conftest.py. Don't we have another module in which we could put this function instead?

swh/loader/tests/conftest.py
66 ↗(On Diff #11752)

We have swh.core.pytest_plugin which declares pytest fixtures to reuse.
Not sure if that qualifies either.

Add tests around assert_last_visit_ok

Note: This does not deal with the location concern yet

Build is green

Patch application report for D3305 (id=11762)

Rebasing onto e7e27a16cd...

Current branch diff-target is up to date.
Changes applied before test
commit 03c67baca063e955f811573d1125eb4833640ede
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 12:36:52 2020 +0200

    package.loader: Retrieve latest snapshot out of the latest visit status
    
    Related to T2310

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

swh/loader/tests/conftest.py
66 ↗(On Diff #11752)

to add tests on it (inclined to do so with mocks)

done

Don't we have another module in which we could put this function instead?

We have swh.core.pytest_plugin which declares pytest fixtures to reuse.
Not sure if that qualifies either.

I think i misread the question the first time.

In loader-core, there is no such module.

We have swh.loader.package.tests.common which technically is fine i guess.
But it will feel a bit awkward to import that namespace in say, dvcs loader
tests.

I guess it's the occasion to bootstrap a swh.loader.tests.common module and
move that function there.

Extract the assert_last_visit_ok function declaration in another diff so the
diff is less big and to the point of migrating loaders to the new storage (and
not a new test helper function :)

Depends on D3321

swh/loader/tests/conftest.py
66 ↗(On Diff #11752)

moved that code to D3321 so that diff is less huge.

Build is green

Patch application report for D3305 (id=11765)

Could not rebase; Attempt merge onto e7e27a16cd...

Updating e7e27a1..59edc43
Fast-forward
 requirements-swh.txt                             |   2 +-
 swh/loader/core/tests/test_loader.py             |   7 +-
 swh/loader/package/archive/tests/test_archive.py |  36 +++----
 swh/loader/package/cran/tests/test_cran.py       |  18 ++--
 swh/loader/package/debian/tests/test_debian.py   |  26 ++---
 swh/loader/package/deposit/tests/test_deposit.py |  20 ++--
 swh/loader/package/loader.py                     |  17 +++-
 swh/loader/package/nixguix/tests/test_nixguix.py |  36 ++++---
 swh/loader/package/npm/tests/test_npm.py         |  29 ++----
 swh/loader/package/pypi/tests/test_pypi.py       |  49 +++------
 swh/loader/package/tests/test_conftest.py        |   2 +-
 swh/loader/package/tests/test_loader.py          |   5 +-
 swh/loader/tests/common.py                       |  53 ++++++++++
 swh/loader/tests/conftest.py                     |   2 +-
 swh/loader/tests/test_common.py                  | 121 +++++++++++++++++++++++
 15 files changed, 287 insertions(+), 136 deletions(-)
 create mode 100644 swh/loader/tests/common.py
 create mode 100644 swh/loader/tests/test_common.py
Changes applied before test
commit 59edc43f213e2cdfc296025f8f9fb86cf0d8923c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 12:36:52 2020 +0200

    package.loader: Retrieve latest snapshot out of the latest visit status
    
    Related to T2310

commit 2b7209f921de49752543d07b06bb3b84afaac6c9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jun 19 17:50:36 2020 +0200

    Add helper function to ensure loader visit are as expected
    
    This bootstraps a swh.loader.tests.common module

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

Remove unneeded modifications (extra line, copyright headers change on no longer modified files)

Build is green

Patch application report for D3305 (id=11766)

Could not rebase; Attempt merge onto e7e27a16cd...

Updating e7e27a1..11cd462
Fast-forward
 requirements-swh.txt                             |   2 +-
 swh/loader/core/tests/test_loader.py             |   7 +-
 swh/loader/package/archive/tests/test_archive.py |  35 +++----
 swh/loader/package/cran/tests/test_cran.py       |  18 ++--
 swh/loader/package/debian/tests/test_debian.py   |  26 ++---
 swh/loader/package/deposit/tests/test_deposit.py |  20 ++--
 swh/loader/package/loader.py                     |  17 +++-
 swh/loader/package/nixguix/tests/test_nixguix.py |  36 ++++---
 swh/loader/package/npm/tests/test_npm.py         |  29 ++----
 swh/loader/package/pypi/tests/test_pypi.py       |  49 +++------
 swh/loader/package/tests/test_conftest.py        |   2 +-
 swh/loader/package/tests/test_loader.py          |   5 +-
 swh/loader/tests/common.py                       |  53 ++++++++++
 swh/loader/tests/conftest.py                     |   2 +-
 swh/loader/tests/test_common.py                  | 121 +++++++++++++++++++++++
 15 files changed, 286 insertions(+), 136 deletions(-)
 create mode 100644 swh/loader/tests/common.py
 create mode 100644 swh/loader/tests/test_common.py
Changes applied before test
commit 11cd46254777745e7910dae69f148719c4f2fe88
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 12:36:52 2020 +0200

    package.loader: Retrieve latest snapshot out of the latest visit status
    
    Related to T2310

commit 2b7209f921de49752543d07b06bb3b84afaac6c9
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jun 19 17:50:36 2020 +0200

    Add helper function to ensure loader visit are as expected
    
    This bootstraps a swh.loader.tests.common module

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

Build is green

Patch application report for D3305 (id=11768)

Could not rebase; Attempt merge onto e7e27a16cd...

Updating e7e27a1..2bd7584
Fast-forward
 requirements-swh.txt                             |   2 +-
 swh/loader/core/tests/test_loader.py             |   7 +-
 swh/loader/package/archive/tests/test_archive.py |  35 +++----
 swh/loader/package/cran/tests/test_cran.py       |  18 ++--
 swh/loader/package/debian/tests/test_debian.py   |  26 ++---
 swh/loader/package/deposit/tests/test_deposit.py |  20 ++--
 swh/loader/package/loader.py                     |  17 +++-
 swh/loader/package/nixguix/tests/test_nixguix.py |  56 ++++++++---
 swh/loader/package/npm/tests/test_npm.py         |  29 ++----
 swh/loader/package/pypi/tests/test_pypi.py       |  49 +++------
 swh/loader/package/tests/test_conftest.py        |   2 +-
 swh/loader/package/tests/test_loader.py          |   5 +-
 swh/loader/tests/common.py                       |  53 ++++++++++
 swh/loader/tests/conftest.py                     |   2 +-
 swh/loader/tests/test_common.py                  | 121 +++++++++++++++++++++++
 15 files changed, 306 insertions(+), 136 deletions(-)
 create mode 100644 swh/loader/tests/common.py
 create mode 100644 swh/loader/tests/test_common.py
Changes applied before test
commit 2bd7584cea939c44951bbcd9bce04a23abeb291c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 12:36:52 2020 +0200

    package.loader: Retrieve latest snapshot out of the latest visit status
    
    Related to T2310

commit f4ebf5c3ac3b5c203e744184ae1434baa018a350
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jun 19 17:50:36 2020 +0200

    Add helper function to ensure loader visit are as expected
    
    This bootstraps a swh.loader.tests.common module

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

douardda added a subscriber: douardda.

Looks good to me but I would have preferred to see the "use assert_last_visit_matches" chuncks, which are a "pure" refactoring step, in a dedicated commit (so split this commit in 2 parts).

This revision is now accepted and ready to land.Jun 22 2020, 11:45 AM

Looks good to me but I would have preferred to see the "use assert_last_visit_matches" chuncks, which are a "pure" refactoring step, in a dedicated commit (so split this commit in 2 parts).

Indeed, it did not start that way though. Also, note that i did improve a bit
since i completely extracted the initial function which does that assertion.
I'll go the extra mile.

I'll split this into 2 commits then.

Build is green

Patch application report for D3305 (id=11800)

Could not rebase; Attempt merge onto f4ebf5c3ac...

Updating f4ebf5c..e4b6ddf
Fast-forward
 swh/loader/core/tests/test_loader.py             |  7 +--
 swh/loader/package/archive/tests/test_archive.py | 35 ++++++---------
 swh/loader/package/cran/tests/test_cran.py       | 18 ++++----
 swh/loader/package/debian/tests/test_debian.py   | 26 +++++------
 swh/loader/package/deposit/tests/test_deposit.py | 20 +++------
 swh/loader/package/loader.py                     | 17 ++++---
 swh/loader/package/nixguix/tests/test_nixguix.py | 56 +++++++++++++++++++-----
 swh/loader/package/npm/tests/test_npm.py         | 29 ++++--------
 swh/loader/package/pypi/tests/test_pypi.py       | 49 +++++++--------------
 swh/loader/package/tests/test_conftest.py        |  2 +-
 swh/loader/package/tests/test_loader.py          |  5 ++-
 swh/loader/tests/conftest.py                     |  2 +-
 12 files changed, 131 insertions(+), 135 deletions(-)
Changes applied before test
commit e4b6ddf8cb30dbe4f5e898edfbc6d3ea38ddb3e2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 14:52:30 2020 +0200

    package.loader: Retrieve latest snapshot out of the latest visit status
    
    Related to T2310

commit a3e63d919e0a4b16f84ad19d2998e1c0fcd7a005
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jun 17 12:36:52 2020 +0200

    tests: Use assert_last_visit_matches
    
    Related to T2310

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

I'll go the extra mile.
I'll split this into 2 commits then.

done

  • D3333 deals with the refactoring of the assertions.
  • This deals with the latest snapshot retrieval which should even be simplified following D3330

Use swh.storage.algos.snapshot.snapshot_get_latest

somehow tests fail, it's a ¯\_(ツ)_/¯ for now.

Build has FAILED

Patch application report for D3305 (id=11812)

Rebasing onto a3e63d919e...

Current branch diff-target is up to date.
Changes applied before test
commit d20d6ac57f3717c87a1b7d2c22de26bedd272c35
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 14:52:30 2020 +0200

    loader: Retrieve latest snapshot with snapshot-get-latest function
    
    Related to T2310

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

tests fail for some reason

because limiting the snapshot's branches size is not a good idea ¯\_(ツ)_/¯ ;)

Fix tests

Build is green

Patch application report for D3305 (id=11816)

Rebasing onto a3e63d919e...

Current branch diff-target is up to date.
Changes applied before test
commit cd5edd90e32ebe3fb502b351e56d0c4f397290d2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 14:52:30 2020 +0200

    loader: Retrieve latest snapshot with snapshot-get-latest function
    
    Related to T2310

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

Build is green

Patch application report for D3305 (id=11822)

Rebasing onto a3e63d919e...

Current branch diff-target is up to date.
Changes applied before test
commit eb444f2cb715b7064ebe9a1eca718fb9829fc702
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 22 14:52:30 2020 +0200

    loader: Retrieve latest snapshot with snapshot-get-latest function
    
    Related to T2310

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

Much easier to review! Thanks.