Fixed unit tests by removing `birthdate` field in seed URs.

BlockchainCommons / seedtool-cli

Cryptographic Seed Tool for the command line

Other

29 stars 16 forks source link

Fixed unit tests by removing `birthdate` field in seed URs. #39

Closed wolfmcnally closed 4 years ago

wolfmcnally commented 4 years ago

Removed birthdate field from generated ur:crypto-seed URs, which was causing unit tests to become invalid. A specific flag to set the birthdate field could be added at a later date.
Bumped version number to 0.9.1

ChristopherA commented 4 years ago

Part of the reason why I liked the birthdate field in some tests was to make sure that if there was data beyond the crypto-seed and the reader didn't know what it was, it would properly ignore it.

Are there still unit tests that still include testing that?

gorazdko commented 4 years ago

Builds and runs on Linux.

Are there still unit tests that still include testing that?

No, this PR completely disables the birthdate generation.

@wolfmcnally please consider reviewing the PR you're building on top of https://github.com/BlockchainCommons/bc-seedtool-cli/pull/37#issuecomment-706590147 first

wolfmcnally commented 4 years ago

Part of the reason why I liked the birthdate field in some tests was to make sure that if there was data beyond the crypto-seed and the reader didn't know what it was, it would properly ignore it.

Are there still unit tests that still include testing that?

The unit tests never tested the birthdate feature. But I did not remove the birthdate field from the CBOR, I simply don't set it now. The problem was that the birthdate field was being set from the system clock, which means that the generated URs weren't truly deterministic. If we want to fully support birthdate in the future, we will simply add a flag that allows either setting the birthdate CBOR field from the system clock or by providing its value on the command line.

wolfmcnally commented 4 years ago

Builds and runs on Linux.

Are there still unit tests that still include testing that?

No, this PR completely disables the birthdate generation.

@wolfmcnally please consider reviewing the PR you're building on top of #37 (comment) first

OK, that's merged. See review comment.

wolfmcnally commented 4 years ago

@gorazdko OK, thank you. That's merged and tested. @ChristopherA ready for review.

gorazdko commented 4 years ago

ACK, builds and runs on windows and linux