Details

Reviewers

vlorentz

Group Reviewers

Reviewers

Maniphest Tasks

T2499: Drop the storage validate proxy used only in tests

Commits

rDSTO6338ad2769f4: Drop validate proxy

Summary

The validate proxy was initially an helper to ease the transition from the use
of dicts towards model objects in "*_add" production endpoints. It was not
removed immediately and grew some behavior it should not have (notably revision
conversion so the comparison within those related tests work).

After finally migrated away from dicts within the tests, we can now drop it [1].

Note that this moves the extra revision conversion behavior from the validate
proxy to those related tests. This extra step will also disappear when we
finally move the "*_get" endpoints to return model objects as well.

Note:

This drops many fixture redefinitions in the process (introduced so we could have that validate proxy).
Remove the "validate" keyword from the get_storage function (so no longer possible to instantiate one [2])

[1] T2994

[2] which practically is the case today, nothing runs on production with it.

Related to T2499

Test Plan

tox

coverage increased a bit, with validate proxy (before):

-------------------------------------------------------------------------------------------------------------------------
TOTAL                                                                                    8411    433   2367    164    93%

===================================================================================== 788 passed, 16 skipped, 1 xfailed, 14 warnings in 352.12s (0:05:52) =====================================================================================
___________________________________________________________________________________________________________________ summary ___________________________________________________________________________________________________________________
  black: commands succeeded
  flake8: commands succeeded
  mypy: commands succeeded
  py3: commands succeeded
  congratulations :)

Now:

TOTAL                                                                                    8247    419   2327    157    94%

===================================================================================== 756 passed, 16 skipped, 1 xfailed, 14 warnings in 340.53s (0:05:40) =====================================================================================
___________________________________________________________________________________________________________________ summary ___________________________________________________________________________________________________________________
  black: commands succeeded
  flake8: commands succeeded
  mypy: commands succeeded
  py3: commands succeeded
  congratulations :)

Diff Detail

Repository

rDSTO Storage manager

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 21 2020, 9:48 AM

Herald added a reviewer: Reviewers. · View Herald TranscriptJul 21 2020, 9:48 AM

ardumont planned changes to this revision.Jul 21 2020, 9:48 AM

ardumont added inline comments.

swh/storage/converters.py
71 ↗	(On Diff #12595)	Tryout
226 ↗	(On Diff #12595)	Tryout
swh/storage/tests/test_retry.py
47–48	This can go away as well.

Build has FAILED

Patch application report for D3579 (id=12595)

Rebasing onto 42ae56d985...

Current branch diff-target is up to date.

Changes applied before test

commit e382b43ad050d32b02470ecfbea63c91bd1225ad
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jul 21 09:42:14 2020 +0200

    wip: Drop validate proxy
    
    It's wip as tests are still red for now
    Currently investigating.
    
    At least, opening the diff now, this is reproducible.
    
    Related to T2499

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

Harbormaster failed remote builds in B13820: Diff 12595!Jul 21 2020, 9:55 AM

Rebase on missing left-over dicts D3580
Drop unnecessary fixtures redefinitions

Note: still failing though

Build has FAILED

Patch application report for D3579 (id=12597)

Could not rebase; Attempt merge onto 42ae56d985...

Updating 42ae56d9..99edfd6c
Fast-forward
 swh/storage/__init__.py              |   3 -
 swh/storage/converters.py            |   6 +-
 swh/storage/tests/conftest.py        |   6 --
 swh/storage/tests/test_api_client.py |   5 +-
 swh/storage/tests/test_retry.py      | 118 +++++++++++-------------------
 swh/storage/tests/test_storage.py    | 136 +----------------------------------
 swh/storage/validate.py              | 124 +-------------------------------
 7 files changed, 51 insertions(+), 347 deletions(-)

Changes applied before test

commit 99edfd6c65746c7b54732f8d571831ea755e3e79
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jul 21 09:42:14 2020 +0200

    wip: Drop validate proxy
    
    It's wip as tests are still red for now
    Currently investigating.
    
    At least, opening the diff now, this is reproducible.
    
    Related to T2499

commit 21cb1b90470eb3898ca08263be04735629276c76
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jul 21 10:12:37 2020 +0200

    tests: Convert left-over dicts to model objects
    
    Related to T2494

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

Harbormaster failed remote builds in B13822: Diff 12597!Jul 21 2020, 10:35 AM

Rebase

ardumont added inline comments.Jul 21 2020, 10:50 AM

swh/storage/tests/test_retry.py
47–48	done (this is no longer targetting the right line)

ardumont planned changes to this revision.Jul 21 2020, 10:52 AM

Build has FAILED

Patch application report for D3579 (id=12601)

Could not rebase; Attempt merge onto 42ae56d985...

Updating 42ae56d9..e921bd22
Fast-forward
 swh/storage/__init__.py              |   3 -
 swh/storage/converters.py            |   6 +-
 swh/storage/tests/conftest.py        |   6 --
 swh/storage/tests/test_api_client.py |   5 +-
 swh/storage/tests/test_retry.py      | 119 +++++++++++-------------------
 swh/storage/tests/test_storage.py    | 136 +----------------------------------
 swh/storage/validate.py              | 124 +-------------------------------
 7 files changed, 52 insertions(+), 347 deletions(-)

Changes applied before test

commit e921bd22f344eb16d168151b518214deed3bbfd2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jul 21 09:42:14 2020 +0200

    wip: Drop validate proxy
    
    It's wip as tests are still red for now
    Currently investigating.
    
    At least, opening the diff now, this is reproducible.
    
    Related to T2499

commit 96b2636703c9a0c8e272c8403f193baffe07d7e5
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jul 21 10:12:37 2020 +0200

    tests: Convert left-over dicts to model objects
    
    Related to T2494

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

Harbormaster failed remote builds in B13826: Diff 12601!Jul 21 2020, 10:58 AM

Finalize diff (diff description update ongoing)

ardumont retitled this revision from wip: Drop validate proxy to tests: Drop validate proxy.Jul 21 2020, 11:22 AM

ardumont edited the summary of this revision. (Show Details)

ardumont edited the test plan for this revision. (Show Details)

ardumont edited the summary of this revision. (Show Details)Jul 21 2020, 11:25 AM

Why do you keep validate.py? it's almost empty now.

And you should change the commit message; it doesn't only affect tests.

swh/storage/tests/test_api_client.py
37–41	shouldn't black fold this?

This revision now requires changes to proceed.Jul 21 2020, 11:26 AM

Build has FAILED

Patch application report for D3579 (id=12603)

Rebasing onto 96b2636703...

Current branch diff-target is up to date.

Changes applied before test

commit 10f43b1850ff3bae444bb43492ae27021c460e6e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jul 21 09:42:14 2020 +0200

    tests: Drop validate proxy
    
    The validate proxy was initially an helper to ease the transition from the use
    of dicts towards model objects in "*_add" production endpoints. It was not
    removed immediately and grew some behavior it should not have (notably revision
    conversion so the comparison within those related tests work).
    
    After finally migrated away from dicts within the tests, we can now drop it [1].
    
    Note that this moves the extra revision conversion behavior from the validate
    proxy to those related tests. This extra step will also disappear when we
    finally move the "*_get" endpoints to return model objects as well.
    
    [1] T2994
    
    Related to T2499

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

Harbormaster failed remote builds in B13828: Diff 12603!Jul 21 2020, 11:30 AM

yeah, right.

I really was focused on making the damn thing go green (which without cass is ;)

Thanks for the feedback heh ;)

Build has failed

yeah, i forgot to run those on cass ;)

swh/storage/tests/test_api_client.py
37–41	i have no idea (but given the time i waited for the commit buffer to happen, like all commits since we have pre-commit, hopefully it did its job ;)

ardumont retitled this revision from tests: Drop validate proxy to Drop validate proxy.Jul 21 2020, 11:33 AM

ardumont edited the summary of this revision. (Show Details)

ardumont edited the summary of this revision. (Show Details)Jul 21 2020, 11:53 AM

ardumont planned changes to this revision.Jul 21 2020, 1:13 PM

Fix cassandra tests: {origin/snapshot}_add resolves iterator now (they did not, it was hidden by validate proxy)
Drop normalize_entity helper which is no longer needed (we compare model objects when needed)
Drop entirely the validate module
Drop some more validation tests

-> Tests should go green

Build is green

Patch application report for D3579 (id=12605)

Rebasing onto e0152b0407...

Current branch diff-target is up to date.

Changes applied before test

commit 6338ad2769f452bd17f7aff7275adec696acb842
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Jul 21 09:42:14 2020 +0200

    Drop validate proxy
    
    The validate proxy was initially an helper to ease the transition from the use
    of dicts towards model objects in "*_add" production endpoints. It was not
    removed immediately and grew some behavior it should not have (notably revision
    conversion so the comparison within those related tests work).
    
    After finally migrated away from dicts within the tests, we can now drop it [1].
    
    Note that this moves the extra revision conversion behavior from the validate
    proxy to those related tests. This extra step will also disappear when we
    finally move the "*_get" endpoints to return model objects as well.
    
    Note:
    - This drops fixture redefinitions in the process (introduced so we could have
    that validate proxy at the time).
    - Remove the "validate" keyword from the get_storage function (so no longer
    possible to instantiate one [2])
    
    [1] T2994
    
    [2] which, practically, is the case today, nothing runs on production with it.
    
    Related to T2499

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

Harbormaster completed remote builds in B13831: Diff 12605.Jul 21 2020, 1:34 PM

ardumont edited the test plan for this revision. (Show Details)Jul 21 2020, 1:55 PM

ardumont retitled this revision from Drop validate proxy to Drop no longer used validate proxy.

ardumont mentioned this in D3582: pytest_plugin: Drop unnecessary back and forth conversion.Jul 21 2020, 9:15 PM

vlorentz accepted this revision.Jul 22 2020, 10:37 AM

vlorentz added inline comments.

swh/storage/tests/test_api_client.py
37–41	aaah, I see, it's because of the trailing comma

This revision is now accepted and ready to land.Jul 22 2020, 10:37 AM

Closed by commit rDSTO6338ad2769f4: Drop validate proxy (authored by ardumont). · Explain WhyJul 22 2020, 10:45 AM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDSTO6338ad2769f4: Drop validate proxy.

Drop no longer used validate proxy
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D3579 (id=12595)

Changes applied before test

Patch application report for D3579 (id=12597)

Changes applied before test

Patch application report for D3579 (id=12601)

Changes applied before test

Patch application report for D3579 (id=12603)

Changes applied before test

Patch application report for D3579 (id=12605)

Changes applied before test

Revision Contents
Changeset List

Diff 12630

swh/storage/init.py

swh/storage/cassandra/storage.py

swh/storage/storage.py

swh/storage/tests/conftest.py

swh/storage/tests/test_api_client.py

swh/storage/tests/test_cassandra.py

swh/storage/tests/test_pytest_plugin.py

swh/storage/tests/test_retry.py

swh/storage/tests/test_storage.py

swh/storage/validate.py

Drop no longer used validate proxyClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D3579 (id=12595)

Changes applied before test

Patch application report for D3579 (id=12597)

Changes applied before test

Patch application report for D3579 (id=12601)

Changes applied before test

Patch application report for D3579 (id=12603)

Changes applied before test

Patch application report for D3579 (id=12605)

Changes applied before test

Revision ContentsChangeset List

Diff 12630

swh/storage/__init__.py

swh/storage/cassandra/storage.py

swh/storage/storage.py

swh/storage/tests/conftest.py

swh/storage/tests/test_api_client.py

swh/storage/tests/test_cassandra.py

swh/storage/tests/test_pytest_plugin.py

swh/storage/tests/test_retry.py

swh/storage/tests/test_storage.py

swh/storage/validate.py

Drop no longer used validate proxy
ClosedPublic
Actions

Revision Contents
Changeset List

swh/storage/init.py