Page MenuHomeSoftware Heritage
Differential D3305

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

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

Details

Reviewers
douardda
Group Reviewers
Reviewers
Maniphest Tasks
T2310: Make origin visits immutable
Commits
rDLDBASEeb444f2cb715: loader: Retrieve latest snapshot with snapshot-get-latest function
Summary

Preparatory work to remove origin visit unused fields

Related to T2310

Depends on D3321

Test Plan

tox

Diff Detail

Event Timeline

ardumont created this revision.Jun 17 2020, 4:24 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJun 17 2020, 4:24 PM
swh-public-ci added a comment.Jun 17 2020, 4:26 PM

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.

Harbormaster completed remote builds in B12917: Diff 11708.Jun 17 2020, 4:26 PM
ardumont mentioned this in D3307: loader: Read snapshot out of the last origin visit status.Jun 17 2020, 5:07 PM
ardumont mentioned this in T2310: Make origin visits immutable.Jun 17 2020, 5:31 PM
ardumont planned changes to this revision.Jun 18 2020, 7:48 AM
ardumont mentioned this in D3313: algos.origin: Open origin-get-latest-visit-status function.Jun 18 2020, 1:45 PM
ardumont updated this revision to Diff 11752.Jun 19 2020, 3:18 PM
  • 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

swh-public-ci added a comment.Jun 19 2020, 3:20 PM

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.

Harbormaster completed remote builds in B12962: Diff 11752.Jun 19 2020, 3:20 PM
ardumont added inline comments.Jun 19 2020, 3:40 PM
swh/loader/tests/conftest.py
66

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 a subscriber: olasd.Jun 19 2020, 3:49 PM
olasd added inline comments.
swh/loader/tests/conftest.py
66

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?

ardumont added inline comments.Jun 19 2020, 4:14 PM
swh/loader/tests/conftest.py
66

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

ardumont updated this revision to Diff 11762.Jun 19 2020, 5:29 PM

Add tests around assert_last_visit_ok

Note: This does not deal with the location concern yet

swh-public-ci added a comment.Jun 19 2020, 5:31 PM

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.

Harbormaster completed remote builds in B12972: Diff 11762.Jun 19 2020, 5:31 PM
ardumont added inline comments.Jun 19 2020, 5:40 PM
swh/loader/tests/conftest.py
66

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.

ardumont updated this revision to Diff 11765.Jun 19 2020, 5:54 PM
ardumont mentioned this in D3321: Add helper function to ensure loader visit/visit-status are as expected.

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

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

ardumont edited the summary of this revision. (Show Details)Jun 19 2020, 5:57 PM
ardumont added a parent revision: D3321: Add helper function to ensure loader visit/visit-status are as expected.
swh-public-ci added a comment.Jun 19 2020, 5:58 PM

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.

Harbormaster completed remote builds in B12975: Diff 11765.Jun 19 2020, 5:58 PM
ardumont updated this revision to Diff 11766.Jun 19 2020, 6:02 PM

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

swh-public-ci added a comment.Jun 19 2020, 6:04 PM

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.

Harbormaster completed remote builds in B12976: Diff 11766.Jun 19 2020, 6:04 PM
ardumont updated this revision to Diff 11768.Jun 19 2020, 6:46 PM

Rebase on top of D3321

swh-public-ci added a comment.Jun 19 2020, 6:49 PM

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.

Harbormaster completed remote builds in B12978: Diff 11768.Jun 19 2020, 6:49 PM
douardda accepted this revision.Jun 22 2020, 11:45 AM
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
ardumont added a comment.Jun 22 2020, 12:33 PM

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.

ardumont updated this revision to Diff 11800.Jun 22 2020, 2:55 PM

Depends on D3333

swh-public-ci added a comment.Jun 22 2020, 2:58 PM

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.

Harbormaster completed remote builds in B13012: Diff 11800.Jun 22 2020, 2:58 PM
ardumont added a comment.Jun 22 2020, 3:00 PM

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
ardumont updated this revision to Diff 11812.Jun 22 2020, 4:47 PM

Use swh.storage.algos.snapshot.snapshot_get_latest

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

swh-public-ci added a comment.Jun 22 2020, 4:49 PM

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

Harbormaster failed remote builds in B13027: Diff 11812!Jun 22 2020, 4:49 PM
ardumont updated this revision to Diff 11816.Jun 22 2020, 5:17 PM

tests fail for some reason

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

Fix tests

swh-public-ci added a comment.Jun 22 2020, 5:19 PM

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.

Harbormaster completed remote builds in B13031: Diff 11816.Jun 22 2020, 5:19 PM
ardumont updated this revision to Diff 11822.Jun 22 2020, 6:38 PM

Bump storage dependency

swh-public-ci added a comment.Jun 22 2020, 6:40 PM

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.

Harbormaster completed remote builds in B13036: Diff 11822.Jun 22 2020, 6:40 PM
douardda accepted this revision.Jun 23 2020, 9:34 AM

Much easier to review! Thanks.

Closed by commit rDLDBASEeb444f2cb715: loader: Retrieve latest snapshot with snapshot-get-latest function (authored by ardumont). · Explain WhyJun 23 2020, 10:33 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDBASEeb444f2cb715: loader: Retrieve latest snapshot with snapshot-get-latest function.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
2 lines--
swh/
loader/
core/
tests/
7 lines97%
Loading...
package/
archive/
tests/
36 lines91%
Loading...
cran/
tests/
18 lines100%
Loading...
debian/
tests/
26 lines100%
Loading...
deposit/
tests/
20 lines100%
Loading...
17 lines94%
Loading...
nixguix/
tests/
36 lines100%
Loading...
npm/
tests/
29 lines98%
Loading...
pypi/
tests/
49 lines100%
Loading...
tests/
2 lines100%
Loading...
5 lines100%
Loading...
tests/
51 lines88%
Loading...

Diff 11752

View Options

requirements-swh.txt

Loading...
View Options

swh/loader/core/tests/test_loader.py

Loading...
View Options

swh/loader/package/archive/tests/test_archive.py

Loading...
View Options

swh/loader/package/cran/tests/test_cran.py

Loading...
View Options

swh/loader/package/debian/tests/test_debian.py

Loading...
View Options

swh/loader/package/deposit/tests/test_deposit.py

Loading...
View Options

swh/loader/package/loader.py

Loading...
View Options

swh/loader/package/nixguix/tests/test_nixguix.py

Loading...
View Options

swh/loader/package/npm/tests/test_npm.py

Loading...
View Options

swh/loader/package/pypi/tests/test_pypi.py

Loading...
View Options

swh/loader/package/tests/test_conftest.py

Loading...
View Options

swh/loader/package/tests/test_loader.py

Loading...
View Options

swh/loader/tests/conftest.py

Loading...
About · Developer information · Legal