Switch to ArdorForum

Thanks for your reply.

Just wondering why you picked those two classes in particular?

I selected these classes since they seemed like the most easy for me to start with, since they appear to have few dependencies and little side effects.

But if you do create unit tests for them, the next logical step will be to rewrite Token.generateToken, Token.parseToken, Hallmark.parseDate and Hallmark.formatDate as those methods look like C and not Java, and with unit tests available it will be safer to clean them up.

I totally agree

To avoid duplicate work, please use the simple junit test framework that is waiting as a pull request on bitbucket, https://bitbucket.org/JeanLucPicard/nxt/pull-request/8/adding-unit-tests-infrastructure-and-some/diff , I have already applied it to the develop branch but it hasn't been released yet.

I will do, thanks for letting me know.

I vote in favour of the proposal.

Thanks for the feedback, I will start on this task and hope that there will be no veto.

Created a pull request https://bitbucket.org/JeanLucPicard/nxt/pull-request/12/proposal-create-unit-tests-for-two-classes

It is based on 1.1.6 plus the commit that contains the simple junit test framework.

can you maybe get JL to comment on it?

You propose changes to the Token and Hallmark classes whose sole reason to exist is to allow those tests. Now there are two new interfaces - EpochTime and Random (not a good name, you know), and instead of the static factory methods generateToken and generateHallmark, you have two new separate factory classes. My philosophy is that design takes precedence over testing, and those changes add complexity that is not needed.

Yes, I understand you cannot test for exact match of token string and hallmark string with some predefined values, unless you control the timestamp and random seed. But you could still test that the generated token (or hallmark) are parsed back to the original token string / hallmark host, weight and date, without checking exact matching of the actual hex strings. For the timestamp test, you could even take the timestamp from Convert.epochTime() before and after the call to generateToken, and check that the resulting token timestamp is in between those values.

The second version have been merged back into Jean-Luc master branch.

https://bitbucket.org/JeanLucPicard/nxt/pull-request/17/proposal-create-unit-tests-for-two-classes/diff

One more comment though, in the test cases that are supposed to fail, you are checking for a very specific exception - StringIndexOutOfBoundsException or BufferUnderflowException. The contract of Hallmark.parseHallmark method does not specify what should happen when the hallmark cannot be parsed, so you shouldn't assume anything more concrete than some type of RuntimeException. Otherwise a future refactored version may throw a different exception, and this would cause the test to fail, and then you would need to change the test.

I know, I did not want to change the production code. I left the test this way, since I feel that the production code should be updated to throw better exceptions in case of invalid arguments.