Nxt Forum

NxtForum Archive => Defunct Committees => Community Funding Committee (CFC) => Technical Development Fund Committee => Topic started by: jonny on July 19, 2014, 10:14:11 am

Title: Proposal: Create unit tests for two classes
Post by: jonny on July 19, 2014, 10:14:11 am
1. Nxt account and userID/contact info for submitter
Forum user: jonny
Nxt account: NXT-3XWR-RVFD-85CK-6JHUH

2. Submission date
Sat Jul 19 2014.

3. A short description of the project with your goals very clearly specified(three sentences max.)
Create unit tests for classes nxt.peer.Hallmark and nxt.Token.

3b. Long description as needed
Automated tests increase developer confidence and quality. As a bonus for me as a new developer is to learn more about the codebase.

4. Specify the target audience
Developers.

5. Budget
1000 NXT for each class tested, total 2000 NXT for both Hallmark and Token.

6. Specify deadlines
Work done 10 days after the proposal is accepted.

7. Metrics need to be specified
Title: Re: Proposal: Create unit tests for two classes
Post by: Jean-Luc on July 19, 2014, 10:45:29 am
Just wondering why you picked those two classes in particular? They haven't had to change in a very long time, so are unlikely to have bugs. 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.

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.

Title: Re: Proposal: Create unit tests for two classes
Post by: jonny on July 19, 2014, 11:45:52 am
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.
Title: Re: Proposal: Create unit tests for two classes
Post by: wesley on July 27, 2014, 07:10:33 pm
Tech Committee:

Can we get a vote on this please or at least an acknowledgement that this is being looked at?

We are always looking for new developers, then when we finally get some, they seem to be ignored... I'm sure this is turning some people off!

Please: A faster acting tech committee is needed. Valuable time is being wasted.
Title: Re: Proposal: Create unit tests for two classes
Post by: LocoMB on July 28, 2014, 06:53:58 am

I vote in favour of the proposal.
Title: Re: Proposal: Create unit tests for two classes
Post by: antanst on July 28, 2014, 07:10:05 am
You have my approval as well, go ahead and get your feet wet jonny.
Title: Re: Proposal: Create unit tests for two classes
Post by: jonny on July 28, 2014, 12:21:11 pm
Thanks for the feedback, I will start on this task and hope that there will be no veto.
Title: Re: Proposal: Create unit tests for two classes
Post by: jonny on August 05, 2014, 05:04:59 pm
Created a pull request https://bitbucket.org/JeanLucPicard/nxt/pull-request/12/proposal-create-unit-tests-for-two-classes (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.
Title: Re: Proposal: Create unit tests for two classes
Post by: LocoMB on August 06, 2014, 10:05:41 am
Created a pull request https://bitbucket.org/JeanLucPicard/nxt/pull-request/12/proposal-create-unit-tests-for-two-classes (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?
Title: Re: Proposal: Create unit tests for two classes
Post by: jonny on August 06, 2014, 10:53:01 am
can you maybe get JL to comment on it?
Yes, I sent Jean-Luc a message now
Title: Re: Proposal: Create unit tests for two classes
Post by: Jean-Luc on August 08, 2014, 03:54:31 pm
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.
Title: Re: Proposal: Create unit tests for two classes
Post by: jonny on August 09, 2014, 04:32:16 pm
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.
Thanks for your feedback, I felt bad when I introduced these changes to the production code.

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.
That sounds like a good starting point, I will try to adapt your ideas and give it another try.
Title: Re: Proposal: Create unit tests for two classes
Post by: jonny on August 17, 2014, 02:28:33 pm
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 (https://bitbucket.org/JeanLucPicard/nxt/pull-request/17/proposal-create-unit-tests-for-two-classes/diff)
Title: Re: Proposal: Create unit tests for two classes
Post by: Jean-Luc on August 17, 2014, 05:20:55 pm
Yes, I merged that in. 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.
Title: Re: Proposal: Create unit tests for two classes
Post by: jonny on August 17, 2014, 05:58:18 pm
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.
elective-stereophonic
elective-stereophonic
assembly
assembly