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

Repository

rDLDBASE Generic VCS/Package Loader

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 13027
Build 19858: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 19857: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	418 ms	Jenkins > .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 ms	Jenkins > .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 ms	Jenkins > .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 ms	Jenkins > .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 ms	Jenkins > .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

ardumont created this revision.Jun 17 2020, 4:24 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptJun 17 2020, 4:24 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

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.

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 ↗	(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 a subscriber: olasd.Jun 19 2020, 3:49 PM

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?

ardumont added inline comments.Jun 19 2020, 4:14 PM

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.

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 ↗	(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.

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.

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

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.

Harbormaster completed remote builds in B12976: Diff 11766.Jun 19 2020, 6:04 PM

Rebase on top of D3321

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

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.

Depends on D3333

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

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

Harbormaster failed remote builds in B13027: Diff 11812!Jun 22 2020, 4:49 PM

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.

Harbormaster completed remote builds in B13031: Diff 11816.Jun 22 2020, 5:19 PM

Bump storage dependency

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

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.

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

Details

Diff Detail

Unit TestsFailed
View All

Event Timeline

Patch application report for D3305 (id=11708)

Changes applied before test

Patch application report for D3305 (id=11752)

Changes applied before test

Patch application report for D3305 (id=11762)

Changes applied before test

Patch application report for D3305 (id=11765)

Changes applied before test

Patch application report for D3305 (id=11766)

Changes applied before test

Patch application report for D3305 (id=11768)

Changes applied before test

Patch application report for D3305 (id=11800)

Changes applied before test

Patch application report for D3305 (id=11812)

Changes applied before test

Patch application report for D3305 (id=11816)

Changes applied before test

Patch application report for D3305 (id=11822)

Changes applied before test

Revision Contents
Changeset List

Diff 11812

swh/loader/package/loader.py

package.loader: Retrieve latest snapshot out of the latest visit statusClosedPublicActions

Details

Diff Detail

Unit TestsFailedView All

Event Timeline

Patch application report for D3305 (id=11708)

Changes applied before test

Patch application report for D3305 (id=11752)

Changes applied before test

Patch application report for D3305 (id=11762)

Changes applied before test

Patch application report for D3305 (id=11765)

Changes applied before test

Patch application report for D3305 (id=11766)

Changes applied before test

Patch application report for D3305 (id=11768)

Changes applied before test

Patch application report for D3305 (id=11800)

Changes applied before test

Patch application report for D3305 (id=11812)

Changes applied before test

Patch application report for D3305 (id=11816)

Changes applied before test

Patch application report for D3305 (id=11822)

Changes applied before test

Revision ContentsChangeset List

Diff 11812

swh/loader/package/loader.py

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

Unit TestsFailed
View All

Revision Contents
Changeset List