Page MenuHomeSoftware Heritage

service: Use latest origin visit status from an origin
ClosedPublic

Authored by ardumont on Thu, Jun 18, 6:43 PM.

Details

Summary

Depends on D3315
Depends on D3313 (storage)

Related to T2310

Test Plan

tox

Diff Detail

Repository
rDWAPPS Web applications
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.Thu, Jun 18, 6:43 PM

Build has FAILED

Patch application report for D3316 (id=11736)

Could not rebase; Attempt merge onto c435bc30e4...

Merge made by the 'recursive' strategy.
 requirements-swh.txt                   |  2 +-
 swh/web/common/service.py              | 18 +++++++++++++-----
 swh/web/tests/api/views/test_origin.py | 14 ++++++++++++--
 swh/web/tests/conftest.py              |  7 ++++---
 swh/web/tests/data.py                  |  3 ++-
 swh/web/tests/strategies.py            | 17 +++++++++--------
 6 files changed, 41 insertions(+), 20 deletions(-)
Changes applied before test
commit f806198f1c07c8fe87f217ac3e829514cd9efb4e
Merge: c435bc30 337c9af5
Author: Jenkins user <jenkins@localhost>
Date:   Thu Jun 18 16:44:36 2020 +0000

    Merge branch 'diff-target' into HEAD

commit 337c9af5a553835157fefa6d0ebefd291fcfa807
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jun 18 18:42:33 2020 +0200

    service: Use latest origin visit status from an origin
    
    Related to T2310

commit ec9d66110224433a7e0193636651a9f60260bbd4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jun 18 17:37:26 2020 +0200

    Migrate to swh.storage.algos.snapshot_get_latest
    
    Related to D3314

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

Build has FAILED

Patch application report for D3316 (id=11743)

Could not rebase; Attempt merge onto 0c7cbf061d...

Merge made by the 'recursive' strategy.
 requirements-swh.txt                   |  2 +-
 swh/web/common/service.py              | 20 +++++++++++++++-----
 swh/web/tests/api/views/test_origin.py | 14 ++++++++++++--
 swh/web/tests/conftest.py              |  7 ++++---
 swh/web/tests/data.py                  |  3 ++-
 swh/web/tests/strategies.py            | 17 +++++++++--------
 6 files changed, 43 insertions(+), 20 deletions(-)
Changes applied before test
commit fe844760abfcb119bd81e2236f1bed1acedd2d69
Merge: 0c7cbf06 a8d64675
Author: Jenkins user <jenkins@localhost>
Date:   Fri Jun 19 08:03:22 2020 +0000

    Merge branch 'diff-target' into HEAD

commit a8d646754d92691b9e7f43a546d40ea820c3e78a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jun 18 18:42:33 2020 +0200

    service: Use latest origin visit status from an origin
    
    Related to T2310

commit d606a719a398d8bb512070dfebe7aeeffbeee2f0
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jun 18 17:37:26 2020 +0200

    Migrate to swh.storage.algos.snapshot_get_latest
    
    Related to D3314

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

anlambert accepted this revision.Fri, Jun 19, 11:51 AM
anlambert added a subscriber: anlambert.

Looks good, I added some nitpick comments.

swh/web/common/service.py
928–929

How about that instead ?

visit, visit_status = visit_and_status if visit_status else ({}, {})
visit_info_dict = {**visit.to_dict(), **visit_status.to_dict()}
swh/web/tests/api/views/test_origin.py
368–371

Maybe you could add a method named origin_visit_get_latest_status in the _ArchiveData class from conftest.py, wrapping the merge of the two dicts returned by swh.storage.algos.origin.origin_get_latest_visit_status ?

This revision is now accepted and ready to land.Fri, Jun 19, 11:51 AM
anlambert added inline comments.Fri, Jun 19, 11:52 AM
swh/web/common/service.py
928–929

Fix typo in my previous comment

visit, visit_status = visit_and_status if visit_and_status else ({}, {})
visit_info_dict = {**visit.to_dict(), **visit_status.to_dict()}
ardumont added inline comments.Fri, Jun 19, 12:03 PM
swh/web/common/service.py
928–929

visit, visit_status = visit_and_status if visit_and_status else ({}, {})

That i did not thought of

visit_info_dict = {visit.to_dict(), visit_status.to_dict()}

yes, i used that initially but we are only interested by the type visit.

The other fields are either redundant (visit, origin) or soon to be removed (status, snapshot, metadata).

:)

Although, if the issue is the multiple return statements, yes, i gather there is no choice but this then.

swh/web/tests/api/views/test_origin.py
368–371

I'll see what i can do ;)

ardumont added inline comments.Fri, Jun 19, 1:15 PM
swh/web/common/service.py
928–929

ah also, that won't work.
When not visit_and_status, we'd have {}.to_dict()which won't work.

anlambert added inline comments.Fri, Jun 19, 1:27 PM
swh/web/common/service.py
928–929

Use this then:

visit, visit_status = (
    {visit_and_status[0].to_dict(), visit_and_status[1].to_dict()}
    if visit_and_status
    else ({}, {})
)
928–929

Typo day ;-)

visit, visit_status = (
    (visit_and_status[0].to_dict(), visit_and_status[1].to_dict())
    if visit_and_status
    else ({}, {})
)
ardumont added inline comments.Fri, Jun 19, 2:29 PM
swh/web/common/service.py
907

I got mislead by the type here.

It should be Optional so a NotFoundExc is raised and properly dealt with in the
upper layer (to return a 404 and not a 500).

If we return the empty dict, the test about the not found use case raises a
500... [1]

[1] Those are not easy to see when hypothesis tests fail... we got a huge

stacktrace about flaky dataset. I got finally out of this one using pdb ¯\_(ツ)_/¯
928–929

typo day

lol ;)

ardumont updated this revision to Diff 11751.Fri, Jun 19, 2:37 PM
  • Adapt according to review
  • Fix type

Note: Tests should go green now

ardumont edited the summary of this revision. (Show Details)Fri, Jun 19, 2:38 PM
ardumont edited the test plan for this revision. (Show Details)
anlambert added inline comments.Fri, Jun 19, 2:40 PM
swh/web/common/service.py
907

yes, hypothesis makes it hard to debug tests.

Passing --hypothesis-verbosity=verbose to pytest usually helps.

Build is green

Patch application report for D3316 (id=11751)

Rebasing onto d3ad49fe39...

Current branch diff-target is up to date.
Changes applied before test
commit a6d6cb345951e583d27fde95cddca4e1feb1a121
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Jun 18 18:42:33 2020 +0200

    service: Use latest origin visit status from an origin
    
    Related to T2310

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