Page MenuHomeSoftware Heritage

Start writing tests for the deposit upload CLI.
ClosedPublic

Authored by vlorentz on Dec 17 2019, 6:53 PM.

Details

Diff Detail

Repository
rDDEP swh-deposit
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Dec 17 2019, 6:53 PM
vlorentz updated this revision to Diff 8764.Dec 17 2019, 7:11 PM

change base

vlorentz edited the summary of this revision. (Show Details)Dec 17 2019, 7:11 PM
vlorentz edited the summary of this revision. (Show Details)

Thanks for this ;)

I'll have a closer look tomorrow.

At first glance, i think you mixed multiple diffs' content with this one though (i see sentry sdk code and the atom stuff moved around which are in other accepted diffs)
;)

ardumont accepted this revision.Dec 19 2019, 8:52 AM
ardumont added a subscriber: moranegg.

I have some non-blocking suggestions.

And thanks again!

swh/deposit/tests/cli/test_client.py
72

why don't you make that a fixture?

@pytest.fixture
def cli_client(mocker):
    mock_client = MagicMock()
    mock_client.service_document.return_value = EXAMPLE_SERVICE_DOCUMENT
    mock_client.deposit_create.return_value = '{"foo": "bar"}'
    mocker.patch(
        'swh.deposit.cli.client._client',
        return_value=mock_client)
    return mock_client

That will unclutter a bit the scenario below.
If that makes sense.

Note:
you can use the factory pattern if you need some different output in the deposit.create.return_value mock.

103
117

jsyk, as @moranegg probably already told you this is kind of a pain in deposit.

The slug is a word used in the sword v2 protocol (it makes no sense to me).
It's needed because it's required in the spec.
Thus, the uuid generation somewhere in the cli here when it's not provided (it needs to be unique).

We decided to use this as external_id, so whenever you see that, think external_id as a unique identifier for the client system...

I don't like the fact that we use both terms but i was a bit worried to unify it under what term and hide that somewhere at the time. Maybe it'd make more sense to rename it once and for all though (later, not right now ;).

This revision is now accepted and ready to land.Dec 19 2019, 8:52 AM
vlorentz marked 2 inline comments as done.Dec 19 2019, 1:42 PM

I find this nicer with the deduplication, thanks.

swh/deposit/tests/cli/test_client.py
117

s/what term/one term/

This revision was automatically updated to reflect the committed changes.