Page MenuHomeSoftware Heritage

test: ensure timestamp in test data are properly typed
ClosedPublic

Authored by douardda on Mar 27 2020, 4:11 PM.

Details

Reviewers
vlorentz
ardumont
Group Reviewers
Reviewers
Summary

according to model declaration, a timestamp must be a dict with 2 keys,
'seconds' and 'microseconds'.

Depends on D2912.

Diff Detail

Repository
rDSTO Storage manager
Branch
valid2
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 11427
Build 17309: tox-on-jenkinsJenkins
Build 17308: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Mar 27 2020, 4:50 PM
vlorentz requested changes to this revision.EditedMar 27 2020, 4:52 PM

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

This revision now requires changes to proceed.Mar 27 2020, 4:52 PM

add a test for date_to_db so the test coverage of this later stays the same

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.

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

This revision now requires changes to proceed.Mar 31 2020, 10:10 AM

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 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.

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,
}])

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.

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.

Yes, I phrased it badly.

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.

Indeed, I forgot that _add methods now take model objects, so nvm

This revision is now accepted and ready to land.Apr 1 2020, 12:21 PM

closed by fcca905a95a262dc5596ff2afd71c657a1bcb522