Page MenuHomeSoftware Heritage

loader.core: check_snapshot: Allow to check using both Snapshot and dict objects
ClosedPublic

Authored by ardumont on Jul 8 2020, 6:40 PM.

Details

Summary

So we can smoothly transition to stop using snapshot as dict (as input).

Next step is making the check_snapshot check first the existence of
objects targetted from the snapshot (first level, release, revision and alias consistency) [1]

(And then check the remaining parts)

Related to T2483

[1] D3478

Test Plan

tox

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Jul 8 2020, 6:40 PM
ardumont edited the summary of this revision. (Show Details)Jul 8 2020, 6:41 PM

Build is green

Patch application report for D3473 (id=12283)

Could not rebase; Attempt merge onto 6dc3da1580...

Updating 6dc3da1..a73a211
Fast-forward
 swh/loader/tests/__init__.py  | 53 +++++++++++++++++---------
 swh/loader/tests/test_init.py | 89 ++++++++++++++++++++++---------------------
 2 files changed, 81 insertions(+), 61 deletions(-)
Changes applied before test
commit a73a21159f3e4749b9adb690b1235805124fd5a4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 8 17:36:21 2020 +0200

    check_snapshot: Allow to check using both Snapshot and dict
    
    So we can smoothly transition to stop using snapshot as dict.
    
    Related to T2483

commit 39ffcde65768aec5bbeda09fc50bf5fb2bf76574
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 8 16:42:31 2020 +0200

    test_init: Use swh_storage fixture
    
    Related to T2481

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

ardumont added inline comments.Jul 8 2020, 6:42 PM
swh/loader/tests/__init__.py
121

oh i wanted to avoid the isinstance call initially but i recall i can't in the end (mypy)...
need to simplify this.

ardumont added inline comments.Jul 8 2020, 6:45 PM
swh/loader/tests/__init__.py
147

Inverted the order to match what we do in other loaders.
Unsure if it's a good idea though.

ardumont updated this revision to Diff 12300.Jul 9 2020, 2:34 PM

Improvments

ardumont edited the summary of this revision. (Show Details)Jul 9 2020, 2:34 PM
anlambert added inline comments.
swh/loader/tests/__init__.py
125

I think you want a f-string here right ?

144

branch type is always bytes no ?

Build was aborted

Patch application report for D3473 (id=12300)

Rebasing onto 39ffcde657...

Current branch diff-target is up to date.
Changes applied before test
commit 294465ac998a44edb5f9029fbaa59def6367a8db
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 8 17:36:21 2020 +0200

    check_snapshot: Allow to check using both Snapshot and dict
    
    So we can smoothly transition to stop using snapshot as dict.
    
    Related to T2483

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

ardumont added inline comments.Jul 9 2020, 2:57 PM
swh/loader/tests/__init__.py
125

yeah (damn!)

144

not necessarily in test.

Initially, to simplify the test writing, we allowed to pass dict-like model object with hashes as hex.
And let that function do the work to convert appropriately for the comparison to actually work.

147

I reverted it.

ardumont updated this revision to Diff 12305.Jul 9 2020, 3:02 PM

Fix forgotten f-string f prefix ¯\_(ツ)_/¯

Build was aborted

Patch application report for D3473 (id=12305)

Rebasing onto 39ffcde657...

Current branch diff-target is up to date.
Changes applied before test
commit 5d02595567ba777aa081a740b86db956e0e8ca2d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 8 17:36:21 2020 +0200

    check_snapshot: Allow to check using both Snapshot and dict
    
    So we can smoothly transition to stop using snapshot as dict.
    
    Related to T2483

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

ardumont updated this revision to Diff 12316.Jul 9 2020, 5:37 PM

Rebase on latest master

Build is green

Patch application report for D3473 (id=12316)

Rebasing onto ed4e0b38c1...

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

    check_snapshot: Allow to check using both Snapshot and dict
    
    So we can smoothly transition to stop using snapshot as dict.
    
    Related to T2483

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

ardumont retitled this revision from check_snapshot: Allow to check using both Snapshot and dict objects to loader.core: check_snapshot: Allow to check using both Snapshot and dict objects.Jul 9 2020, 7:01 PM
ardumont edited the summary of this revision. (Show Details)
douardda requested changes to this revision.Jul 10 2020, 10:19 AM
douardda added a subscriber: douardda.
douardda added inline comments.
swh/loader/tests/__init__.py
112–113

expected_snapshot or snapshot ?

121

Since we try to go full "model object everywhere", why not use the Snapshot within this test rather than the dict form?

124–125

I think this is unnecessary (the else case). This is testing code, if something incorrect is given, it will fail anyway.

This revision now requires changes to proceed.Jul 10 2020, 10:19 AM
ardumont added inline comments.Jul 10 2020, 10:25 AM
swh/loader/tests/__init__.py
112–113

It's the expected_snapshot but i renamed it so it can reuse the name internally without mypy annoying me with its type.

I forgot to rename the docstring, i'll fix that.

121

Because I went with the existing code which already manipulated the dict.
(also i try to keep diff small somehow, i gather modifying more would have meant more code)

124–125

might as well be explicit?
I'm annoyed when i don't understand immediately the assertion failure (and it happens).

ardumont edited the summary of this revision. (Show Details)Jul 10 2020, 11:10 AM
ardumont added inline comments.Jul 10 2020, 11:13 AM
swh/loader/tests/__init__.py
121

And it means more code, for example, i'd need to code the symmetric encode_target here so i can Snapshot.from_dict the all thing.

ardumont added inline comments.Jul 10 2020, 11:26 AM
swh/loader/tests/__init__.py
121

And then convert the snapshot dict returned from the storage endpoint...
(which i think started it all).

ardumont added inline comments.Jul 10 2020, 11:55 AM
swh/loader/tests/__init__.py
121

context: trying to do as you proposed.

Even though unfinished, i found the result quite unreadable in terms of error message...
I'm not sure i want to do that...

ardumont updated this revision to Diff 12346.Jul 10 2020, 12:21 PM

Rework according to remarks

Build is green

Patch application report for D3473 (id=12346)

Rebasing onto ed4e0b38c1...

Current branch diff-target is up to date.
Changes applied before test
commit cb9647085a1d5f41024255089d2282de5ecbe11b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Jul 10 12:21:27 2020 +0200

    Rework according to remarks

commit b5ccfd7ae14ad8f415417281e4e135779a839a61
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 8 17:36:21 2020 +0200

    check_snapshot: Allow to check using both Snapshot and dict
    
    So we can smoothly transition to stop using snapshot as dict.
    
    Related to T2483

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

ardumont updated this revision to Diff 12348.Jul 10 2020, 12:44 PM

Squash commits

douardda accepted this revision.Jul 10 2020, 12:44 PM
This revision is now accepted and ready to land.Jul 10 2020, 12:44 PM

Build is green

Patch application report for D3473 (id=12348)

Rebasing onto ed4e0b38c1...

Current branch diff-target is up to date.
Changes applied before test
commit 72590f99808881c8f0beaececad944e070a3de9f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 8 17:36:21 2020 +0200

    check_snapshot: Allow to check using both Snapshot and dict
    
    So we can smoothly transition to stop using snapshot as dict.
    
    Related to T2483

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

As agreed on irc, i'll keep the snapshot dict conversion for now.
When done with the initial goal D3478, i'll migrate the tests
so they stop using dict and start using Snasphot objects instead.