Page MenuHomeSoftware Heritage
Differential D3579

Drop no longer used validate proxy
ClosedPublic
Actions

Authored by ardumont on Jul 21 2020, 9:48 AM.

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
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13820
Build 21179: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 21178: arc lint + arc unit

Unit TestsFailed
View All

TimeTest
65 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.tests.test_api_client.TestStorage::test_revision_get
self = <swh.storage.tests.test_api_client.TestStorage object at 0x7f19491b4208> swh_storage = <RemoteStorage url=mock://example.com/> sample_data_model = {'authority': [MetadataAuthority(type=<MetadataAuthorityType.DEPOSIT_CLIENT: 'deposit_client'>, url='http://hal.inria....11', length=7, status='visible', data=b'424242\n', ctime=datetime.datetime(2019, 12, 1, 0, 0, tzinfo=tzlocal()))], ...}
63 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.tests.test_api_client.TestStorage::test_revision_get_no_parents
self = <swh.storage.tests.test_api_client.TestStorage object at 0x7f1949f69550> swh_storage = <RemoteStorage url=mock://example.com/> sample_data_model = {'authority': [MetadataAuthority(type=<MetadataAuthorityType.DEPOSIT_CLIENT: 'deposit_client'>, url='http://hal.inria....11', length=7, status='visible', data=b'424242\n', ctime=datetime.datetime(2019, 12, 1, 0, 0, tzinfo=tzlocal()))], ...}
97 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.tests.test_api_client.TestStorage::test_revision_get_order
self = <swh.storage.tests.test_api_client.TestStorage object at 0x7f194977cbe0> swh_storage = <RemoteStorage url=mock://example.com/> sample_data_model = {'authority': [MetadataAuthority(type=<MetadataAuthorityType.DEPOSIT_CLIENT: 'deposit_client'>, url='http://hal.inria....11', length=7, status='visible', data=b'424242\n', ctime=datetime.datetime(2019, 12, 1, 0, 0, tzinfo=tzlocal()))], ...}
82 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.tests.test_api_client.TestStorage::test_revision_log
self = <swh.storage.tests.test_api_client.TestStorage object at 0x7f1949b68438> swh_storage = <RemoteStorage url=mock://example.com/> sample_data_model = {'authority': [MetadataAuthority(type=<MetadataAuthorityType.DEPOSIT_CLIENT: 'deposit_client'>, url='http://hal.inria....11', length=7, status='visible', data=b'424242\n', ctime=datetime.datetime(2019, 12, 1, 0, 0, tzinfo=tzlocal()))], ...}
61 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.storage.tests.test_api_client.TestStorage::test_revision_log_with_limit
self = <swh.storage.tests.test_api_client.TestStorage object at 0x7f19499db518> swh_storage = <RemoteStorage url=mock://example.com/> sample_data_model = {'authority': [MetadataAuthority(type=<MetadataAuthorityType.DEPOSIT_CLIENT: 'deposit_client'>, url='http://hal.inria....11', length=7, status='visible', data=b'424242\n', ctime=datetime.datetime(2019, 12, 1, 0, 0, tzinfo=tzlocal()))], ...}
View Full Test Results (23 Failed · 733 Passed · 17 Skipped)

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

Tryout

226

Tryout

swh/storage/tests/test_retry.py
51

This can go away as well.

swh-public-ci added a comment.Jul 21 2020, 9:55 AM

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
ardumont updated this revision to Diff 12597.Jul 21 2020, 10:21 AM
  • Rebase on missing left-over dicts D3580
  • Drop unnecessary fixtures redefinitions

Note: still failing though

swh-public-ci added a comment.Jul 21 2020, 10:35 AM

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
ardumont updated this revision to Diff 12601.EditedJul 21 2020, 10:45 AM

Rebase

ardumont added inline comments.Jul 21 2020, 10:50 AM
swh/storage/tests/test_retry.py
51

done (this is no longer targetting the right line)

ardumont planned changes to this revision.Jul 21 2020, 10:52 AM
swh-public-ci added a comment.Jul 21 2020, 10:58 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
ardumont updated this revision to Diff 12603.Jul 21 2020, 11:22 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
vlorentz requested changes to this revision.Jul 21 2020, 11:26 AM
vlorentz added a subscriber: vlorentz.

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
swh-public-ci added a comment.Jul 21 2020, 11:30 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
ardumont added a comment.Jul 21 2020, 11:32 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
ardumont updated this revision to Diff 12605.EditedJul 21 2020, 1:26 PM
ardumont edited the summary of this revision. (Show Details)
  • 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

swh-public-ci added a comment.Jul 21 2020, 1:33 PM

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.

Revision Contents
Changeset List

PathSize
swh/
storage/
3 lines
6 lines
tests/
6 lines
5 lines
107 lines
136 lines
124 lines

Diff 12595

View Options

swh/storage/__init__.py

Loading...
View Options

swh/storage/converters.py

Loading...
View Options

swh/storage/tests/conftest.py

Loading...
View Options

swh/storage/tests/test_api_client.py

Loading...
View Options

swh/storage/tests/test_retry.py

Loading...
View Options

swh/storage/tests/test_storage.py

Loading...
View Options

swh/storage/validate.py

Loading...
About · Developer information · Legal