Page MenuHomeSoftware Heritage

storage*: Allow origin-visit-get-latest to filter on type
ClosedPublic

Authored by ardumont on Jun 18 2020, 10:13 AM.

Diff Detail

Repository
rDSTO Storage manager
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12931
Build 19697: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 19696: arc lint + arc unit

Event Timeline

swh/storage/tests/test_storage.py
1797

mmmph, wrong location, that should go in the test around origin-visit-get-latest (we are in the origin-visit-status-add test here ;)

Build is green

Patch application report for D3311 (id=11720)

Rebasing onto 822d96b580...

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

    storage*: Allow origin-visit-get-latest to filter on type

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

  • Move assertion on origin-visit-get-latest for type to the right test
  • Add types sample in the interface docstring for the endpoint
  • (Rework testt origin-visit-get-latest to deal with model object)

Drop left over assertion added by mistake

Build was aborted

Patch application report for D3311 (id=11721)

Rebasing onto 822d96b580...

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

    storage*: Allow origin-visit-get-latest to filter on type

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

Build is green

Patch application report for D3311 (id=11722)

Rebasing onto 822d96b580...

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

    storage*: Allow origin-visit-get-latest to filter on type

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

I'm on the fence about accepting this with the mega test as is; The logic is sound, so I'm not too worried about the test being... a lot. But still :D

swh/storage/db.py
676

= None to avoid an api break?

swh/storage/tests/test_storage.py
2003

This test is now 180 lines long.

Could we consider splitting it into more readable chunks? I think comparing the results for various filters should be split into separate tests, instead of done sequentially in a single huge test (that I have a hard time following).

swh/storage/db.py
676

missed that one, yes, thanks

swh/storage/tests/test_storage.py
2003

granted!

I have a hard time myself.

swh/storage/db.py
676

that's db.py, not an API

677–678

these don't need to have defaults either

swh/storage/interface.py
869

What about type: Optional[Iterable[str]] = None? I don't see any immediate use for it, but it's free and might save a small breaking API change later

swh/storage/db.py
677–678

ah yes, we are in db, not in the api.

So, that means, i can remove all defaults since it will be called from storage with them set (except for cur baybe).
Is that what you meant?

swh/storage/interface.py
869

well, i'm unsure about this.

I see what you mean but also i see yagni... (maybe) and i'd like also to land
this so i can finally close the immutable origin visit crusade ¯\_(ツ)_/¯

but it's free...

Like Alice Cooper said, "nothing is free" (as in paid-service)! :D

I'll have to rewrite stuff in all backends again and tests for it and those
tests are slow...

Oh and also right now i already need to rewrite to extract tests in another
method... etc...

swh/storage/tests/test_storage.py
2003

I'll start by getting the type filtering test in another test for that diff as that makes sense here.

  • Split "filter with type" tests into their own test method
  • Drop unneeded defaults to db.origin-visit-get-latest

Note:

Regarding the filtering on type`S`, even though the suggestion sounds fair, I'm
tending towards not wanting to modify it (from type: Optional[str] to types:
Optional[Iterable[str]])

Because, right now i don't see a use for it. But also because we are only on
the origin_visit_get_latest endpoint here. If we want more type filtering, it
might make more sense on broader endpoints, origin_visit_get(_all?) or
something.

Drop removal of assertions in wrong test (<- see the length of those tests
confuse me as well to the point where i took wrong instructions out ¯\_(ツ)_/¯)

Build is green

Patch application report for D3311 (id=11724)

Rebasing onto 822d96b580...

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

    storage*: Allow origin-visit-get-latest to filter on type

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

Build is green

Patch application report for D3311 (id=11726)

Rebasing onto 822d96b580...

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

    storage*: Allow origin-visit-get-latest to filter on type

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

This revision is now accepted and ready to land.Jun 18 2020, 1:55 PM