Page MenuHomeSoftware Heritage

Add tests for conflict resolution functions
ClosedPublic

Authored by aeviso on Nov 26 2021, 5:08 PM.

Details

Diff Detail

Event Timeline

Build is green

Patch application report for D6697 (id=24331)

Could not rebase; Attempt merge onto 579c3bd35e...

Updating 579c3bd..40950ae
Fast-forward
 .gitignore                                       |   4 +-
 docs/storage/remote.rst                          | 720 +++++++++++++++++++++++
 mypy.ini                                         |   3 +
 pytest.ini                                       |   2 +
 requirements-test.txt                            |   1 +
 requirements.txt                                 |   1 +
 swh/provenance/__init__.py                       |   8 +
 swh/provenance/api/client.py                     | 463 +++++++++++++++
 swh/provenance/api/server.py                     | 646 +++++++++++++++++++-
 swh/provenance/cli.py                            |  31 +-
 swh/provenance/tests/conftest.py                 |  24 +-
 swh/provenance/tests/test_conflict_resolution.py | 143 +++++
 swh/provenance/util.py                           |   5 +
 tox.ini                                          |   3 +-
 14 files changed, 2040 insertions(+), 14 deletions(-)
 create mode 100644 docs/storage/remote.rst
 create mode 100644 swh/provenance/tests/test_conflict_resolution.py
Changes applied before test
commit 40950aedd630c874cfefdfeffe14fc3f56c0e786
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Tue Sep 28 10:01:43 2021 +0200

    Add tests for conflict resolution functions

commit 5f1d1b13504ee534f3726a8002d7d3a4609ac1c3
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Tue Sep 21 16:13:53 2021 +0200

    Add support for remote backend on existing storage tests

commit 84a62793d5de0b51aa7706b04a773366f5411391
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Fri Aug 20 12:21:27 2021 +0200

    Add new RabbitMQ-based client/server API
    
    Write methods in the `ProvenanceStorageInterface` are called through a server that
    guarantees conflict-free writings to the underlying database.
    
    Read methods are called directly from the client to avoid RCP overhead for reads.
    
    The server spawns multiple sub-processes to handle independent requests concurrently.

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

douardda added a subscriber: douardda.

Fine for me; these tests could be written as parametrized tests (https://docs.pytest.org/en/6.2.x/parametrize.html#parametrize-basics) but not a big deal.

This revision is now accepted and ready to land.Nov 29 2021, 10:04 AM

When I did so with other tests your claim was that they were too difficult to follow and that tests should be as explicit as possible. These ones have a declarative function name for each case. What's the criteria after all?

When I did so with other tests your claim was that they were too difficult to follow and that tests should be as explicit as possible. These ones have a declarative function name for each case. What's the criteria after all?

I don't know why you are talking about this. I am talking about something like this:

DATE = datetime.fromtimestamp(1000000000)
ONE_SEC = timedelta(seconds=1)
HASH = b"20329687bb9c1231a7e05afe86160343ad49b494"
RESOLVED_DATES = [
    (((HASH, ), (HASH, DATE), (HASH, None)), {HASH: DATE}),
    ((HASH, DATE), (HASH, DATE+ONE_SEC)), {HASH: DATE}),
]
@pytest.mark.parametrize("input,expected", RESOLVED_DATES) 
def test_resolve_dates() -> None:
    assert resolve_dates(input) == expected

I believe we can agree that this is not difficult to follow and read.

Also note that I accepted this diff, so I really don't understand what you are looking for with this...

DATE = datetime.fromtimestamp(1000000000)
ONE_SEC = timedelta(seconds=1)
HASH = b"20329687bb9c1231a7e05afe86160343ad49b494"
RESOLVED_DATES = [
    (((HASH, ), (HASH, DATE), (HASH, None)), {HASH: DATE}),
    ((HASH, DATE), (HASH, DATE+ONE_SEC)), {HASH: DATE}),
]
@pytest.mark.parametrize("input,expected", RESOLVED_DATES) 
def test_resolve_dates() -> None:
    assert resolve_dates(input) == expected

I believe we can agree that this is not difficult to follow and read.

I've never said this is difficult to read. Actually that's more in line to the previous test that I've originally submitted (for other methods) and I then I had to write in a more "explicit" on demand.

Also note that I accepted this diff, so I really don't understand what you are looking for with this...

What am I looking for? Simple, I'm trying to understand what the criteria are, to avoid having to rework the same stuff over and over again.
For now I'll stay with the more "explicit" form as that's what we've agreed before and I have several other test in the same style.

Build is green

Patch application report for D6697 (id=24360)

Could not rebase; Attempt merge onto 579c3bd35e...

Updating 579c3bd..e4d0976
Fast-forward
 .gitignore                                       |   4 +-
 docs/storage/remote.rst                          | 720 +++++++++++++++++++++++
 mypy.ini                                         |   3 +
 pytest.ini                                       |   2 +
 requirements-test.txt                            |   1 +
 requirements.txt                                 |   1 +
 swh/provenance/__init__.py                       |   8 +
 swh/provenance/api/client.py                     | 466 +++++++++++++++
 swh/provenance/api/server.py                     | 666 ++++++++++++++++++++-
 swh/provenance/cli.py                            |  31 +-
 swh/provenance/tests/conftest.py                 |  24 +-
 swh/provenance/tests/test_conflict_resolution.py | 158 +++++
 swh/provenance/util.py                           |   5 +
 tox.ini                                          |   3 +-
 14 files changed, 2078 insertions(+), 14 deletions(-)
 create mode 100644 docs/storage/remote.rst
 create mode 100644 swh/provenance/tests/test_conflict_resolution.py
Changes applied before test
commit e4d097695294def8d809fbce6455b3c8712735dc
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Tue Sep 28 10:01:43 2021 +0200

    Add tests for conflict resolution functions

commit 4eecec5edb9139e945912ff39c7d0f412d3c9977
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Tue Sep 21 16:13:53 2021 +0200

    Add support for remote backend on existing storage tests

commit f8411a522e937b961d7fc8270c653d83039268fd
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Fri Aug 20 12:21:27 2021 +0200

    Add new RabbitMQ-based client/server API
    
    Write methods in the `ProvenanceStorageInterface` are called through a server that
    guarantees conflict-free writings to the underlying database.
    
    Read methods are called directly from the client to avoid RCP overhead for reads.
    
    The server spawns multiple sub-processes to handle independent requests concurrently.

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

Build is green

Patch application report for D6697 (id=24368)

Could not rebase; Attempt merge onto 579c3bd35e...

Updating 579c3bd..6306b44
Fast-forward
 .gitignore                                       |   4 +-
 docs/storage/remote.rst                          | 724 +++++++++++++++++++++++
 mypy.ini                                         |   3 +
 pytest.ini                                       |   2 +
 requirements-test.txt                            |   1 +
 requirements.txt                                 |   1 +
 swh/provenance/__init__.py                       |   8 +
 swh/provenance/api/client.py                     | 466 +++++++++++++++
 swh/provenance/api/server.py                     | 666 ++++++++++++++++++++-
 swh/provenance/cli.py                            |  31 +-
 swh/provenance/tests/conftest.py                 |  24 +-
 swh/provenance/tests/test_conflict_resolution.py | 158 +++++
 swh/provenance/util.py                           |   5 +
 tox.ini                                          |  38 +-
 14 files changed, 2117 insertions(+), 14 deletions(-)
 create mode 100644 docs/storage/remote.rst
 create mode 100644 swh/provenance/tests/test_conflict_resolution.py
Changes applied before test
commit 6306b44896c1775aef49b8840464efd80df9e648
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Tue Sep 28 10:01:43 2021 +0200

    Add tests for conflict resolution functions

commit e649205e258980cafbeb1db7d18f9c6efb1a8e76
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Tue Sep 21 16:13:53 2021 +0200

    Add support for remote backend on existing storage tests

commit a6cc3e4daf228ce9c124712b93c4749b16e65ce1
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Fri Aug 20 12:21:27 2021 +0200

    Add new RabbitMQ-based client/server API
    
    Write methods in the `ProvenanceStorageInterface` are called through a server that
    guarantees conflict-free writings to the underlying database.
    
    Read methods are called directly from the client to avoid RCP overhead for reads.
    
    The server spawns multiple sub-processes to handle independent requests concurrently.

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