Build is green
See https://jenkins.softwareheritage.org/job/DSTO/job/tox/1113/ for more details.

Build is green

Patch application report for D2914 (id=10386)

Rebasing onto 623a1b75cb...

Current branch diff-target is up to date.

Changes applied before test

commit ca06d1bf2d43ff9796810a414b9884844e2aea6f
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed Mar 25 16:08:06 2020 +0100

    test: ensure timestamp in test data are properly typed
    
    according to model declaration, a timestamp must be a dict with 2 keys,
    'seconds' and 'microseconds'.
    
    Also add a few more tests for the date_to_db helper function so that the
    test coverage of this later remains.

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

In D2914#70601, @vlorentz wrote:

it also misses tests to check date_to_db is used in release_to_db and revision_to_db

ok (but plz give all the infos/comments at once!)

In D2914#70849, @douardda wrote:

In D2914#70601, @vlorentz wrote:

it also misses tests to check date_to_db is used in release_to_db and revision_to_db

ok (but plz give all the infos/comments at once!)

in fact no, not OK, I don't understand what you want me to check.

In D2914#70849, @douardda wrote:

In D2914#70601, @vlorentz wrote:

it also misses tests to check date_to_db is used in release_to_db and revision_to_db

ok (but plz give all the infos/comments at once!)

It was a comment on your new version on the diff.

in fact no, not OK, I don't understand what you want me to check.

I wanted you to add tests checking that int timestamps in revision/release were normalized (though I badly phrased it in my first comment), but you only added tests directly on date_to_db.

There is currently no test checking what happens when we do:

storage.release_add([{
    'id': b'87659012345678901234',
    'name': b'v0.0.1',
    'author': {
        'name': b'olasd',
        'email': b'nic@olasd.fr',
        'fullname': b'olasd <nic@olasd.fr>',
    },
    'date': {
        'timestamp': 1234567890,
        'offset': 42,
        'negative_utc': False,
    },
    'target': b'43210987654321098765',
    'target_type': 'revision',
    'message': b'synthetic release',
    'synthetic': True,
}])

In D2914#70906, @vlorentz wrote:

In D2914#70849, @douardda wrote:

In D2914#70601, @vlorentz wrote:

it also misses tests to check date_to_db is used in release_to_db and revision_to_db

ok (but plz give all the infos/comments at once!)

It was a comment on your new version on the diff.

Well, the original comment was

This partially removes test coverage of swh.storage.converters.date_to_db.

which I understood as "you need to add a test for date_to_db" not "you need to add tests for all functions that use this date_to_db" function.

in fact no, not OK, I don't understand what you want me to check.

I wanted you to add tests checking that int timestamps in revision/release were normalized (though I badly phrased it in my first comment), but you only added tests directly on date_to_db.

How is this "normalized"? Is it documented somehow? To me, the model is the final description (and thus documentation) of what data structures are expected. And what we currently have/do is the result of what the code currently support rather than a specified behavior.

There is currently no test checking what happens when we do:

storage.release_add([{
    'id': b'87659012345678901234',
    'name': b'v0.0.1',
    'author': {
        'name': b'olasd',
        'email': b'nic@olasd.fr',
        'fullname': b'olasd <nic@olasd.fr>',
    },
    'date': {
        'timestamp': 1234567890,
        'offset': 42,
        'negative_utc': False,
    },
    'target': b'43210987654321098765',
    'target_type': 'revision',
    'message': b'synthetic release',
    'synthetic': True,
}])

Well one idea of unit tests is to not have to test each and every possible execution path of the whole code base... date_to_db is used in release_to_db, since date_to_db is checked, I do not see the real interest of adding a specific test case for this.

Also, one could argue that timestamps in a release given with null microseconds given as a dict were not tested before either.

And we do want to go to a properly typed usage of structured data in all our APIs. For example it's you who advocated for not using a default value for negative_utc to make sure API users do explicitly specify this flag value.