Page MenuHomeSoftware Heritage
Differential D3473

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

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

Details

Reviewers
douardda
Group Reviewers
Reviewers
Maniphest Tasks
T2483: tests: Make check-snapshot utility test function recursively check targetted object exists
Commits
rDLDBASE72590f998088: check_snapshot: Allow to check using both Snapshot and dict
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

Event Timeline

ardumont created this revision.Jul 8 2020, 6:40 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptJul 8 2020, 6:40 PM
ardumont edited the summary of this revision. (Show Details)Jul 8 2020, 6:41 PM
swh-public-ci added a comment.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.

Harbormaster completed remote builds in B13502: Diff 12283.Jul 8 2020, 6:41 PM
ardumont added inline comments.Jul 8 2020, 6:42 PM
swh/loader/tests/__init__.py
120

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
143

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 a subscriber: anlambert.Jul 9 2020, 2:50 PM
anlambert added inline comments.
swh/loader/tests/__init__.py
124

I think you want a f-string here right ?

140

branch type is always bytes no ?

swh-public-ci added a comment.Jul 9 2020, 2:55 PM

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

Harbormaster failed remote builds in B13519: Diff 12300!Jul 9 2020, 2:55 PM
ardumont added inline comments.Jul 9 2020, 2:57 PM
swh/loader/tests/__init__.py
124

yeah (damn!)

140

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.

143

I reverted it.

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

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

swh-public-ci added a comment.Jul 9 2020, 3:23 PM

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

Harbormaster failed remote builds in B13524: Diff 12305!Jul 9 2020, 3:23 PM
ardumont updated this revision to Diff 12316.Jul 9 2020, 5:37 PM

Rebase on latest master

swh-public-ci added a comment.Jul 9 2020, 5:39 PM

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.

Harbormaster completed remote builds in B13537: Diff 12316.Jul 9 2020, 5:39 PM
ardumont added a child revision: D3478: loader.core: Make check_snapshot ensure snapshot object references exist.Jul 9 2020, 6:43 PM
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

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

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

swh-public-ci added a comment.Jul 10 2020, 12:23 PM

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.

Harbormaster completed remote builds in B13569: Diff 12346.Jul 10 2020, 12:23 PM
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
swh-public-ci added a comment.Jul 10 2020, 12:46 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.

Harbormaster completed remote builds in B13571: Diff 12348.Jul 10 2020, 12:46 PM
ardumont added a comment.Jul 10 2020, 12:47 PM

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.

Closed by commit rDLDBASE72590f998088: check_snapshot: Allow to check using both Snapshot and dict (authored by ardumont). · Explain WhyJul 10 2020, 12:50 PM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDLDBASE72590f998088: check_snapshot: Allow to check using both Snapshot and dict.
ardumont mentioned this in D3478: loader.core: Make check_snapshot ensure snapshot object references exist.Jul 10 2020, 12:54 PM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
loader/
tests/
53 lines100%
Loading...
69 lines100%
Loading...

Diff 12316

View Options

swh/loader/tests/__init__.py

Loading...
View Options

swh/loader/tests/test_init.py

Loading...
About · Developer information · Legal