Page MenuHomeSoftware Heritage

PersistentId constructor parameters validation
ClosedPublic

Authored by DanSeraf on Tue, Oct 1, 5:42 PM.

Details

Summary

Validation checks moved from test_persistent_identifier to PersistentId constuctor.

In test_identifiers the validation error messages in the function test_parse_persistent_identifier_parsing_error should be changed according to the error messages raised in the constructor. In the function test_persistent_identifier there are two cases which the version of the pid is 2, the test fails because the version should be 1.

Diff Detail

Repository
rDMOD Data Model
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

DanSeraf created this revision.Tue, Oct 1, 5:42 PM
DanSeraf retitled this revision from PersistentId parameters validation to PersistentId constructor parameters validation.Tue, Oct 1, 5:49 PM
DanSeraf edited the summary of this revision. (Show Details)Tue, Oct 1, 5:55 PM
DanSeraf edited the test plan for this revision. (Show Details)
DanSeraf edited the summary of this revision. (Show Details)
zack requested changes to this revision.Wed, Oct 2, 10:14 AM
zack added a subscriber: zack.

Hheya, thanks a lot for this contribution !

Before discussing the merits of the patch, can you make sure the CI passes? currently it does not (see linked jenkins report).

For what is worth, I think the current tests that checks the exception string are too strict (both the preexisting ones and the new one you are adding). Only checking the exception type (using assertRaises instead of assertRaisesRegex) should be enough and will be less brittle in the future.

This revision now requires changes to proceed.Wed, Oct 2, 10:14 AM
DanSeraf updated this revision to Diff 6937.Wed, Oct 2, 11:50 AM

test_identifiers: checking exception type instead of exception string

douardda added inline comments.
swh/model/identifiers.py
803

why this chunk? If using the (kinda private) _make method is bothering you, I would rather use return PersistentId(*pid_data)

swh/model/tests/test_identifiers.py
771–773

There might be a good reason for this, but I find it very suspicious to have to change these expected values here.

DanSeraf added inline comments.Thu, Oct 3, 6:43 PM
swh/model/identifiers.py
803

Sorry for the mistake, i will update it. The problem is that when the persistent identifier is built using the _make method the ValidationError in the constructor is not raised

swh/model/tests/test_identifiers.py
771–773

I don't know if different pid versions are accepted. According to the parse_persistent_identifier function, the pid version must be 1

zack requested changes to this revision.Fri, Oct 4, 8:29 AM
zack added inline comments.
swh/model/identifiers.py
803

Yeah, just use the * form here, which is more concise/elegant, and I think we're good with this diff. Thanks!

swh/model/tests/test_identifiers.py
771–773

It looks like these tests were testing hypothetical identifiers in schema version 2, that cannot really exist right now. With proper validation they cannot be created anymore, and that's correct AFAICT.

This revision now requires changes to proceed.Fri, Oct 4, 8:29 AM
zack added a comment.Fri, Oct 4, 8:30 AM

oh, and remember to add a "Closes: T1986" line to the diff description (and commit), so that there is a link with the corresponding issue

zack accepted this revision.Fri, Oct 4, 7:06 PM
This revision is now accepted and ready to land.Fri, Oct 4, 7:06 PM
This revision was automatically updated to reflect the committed changes.
zack added a comment.Fri, Oct 4, 7:13 PM

thanks a lot @DanSeraf!, this has now been accepted, landed to master, and you've been added to the CONTRIBUTORS file :-)

thanks a lot @DanSeraf!, this has now been accepted, landed to master, and you've been added to the CONTRIBUTORS file :-)

thank you for adding me in the CONTRIBUTORS file!
I'm glad to have contributed to this awesome project :)