elective-stereophonic
elective-stereophonic
Proposal: Create unit tests for two classes
singapore
Please login or register.

Login with username, password and session length
Advanced search  

News:

Latest Nxt Client: Nxt 1.11.15

Author Topic: Proposal: Create unit tests for two classes  (Read 3875 times)

jonny

  • Jr. Member
  • **
  • Karma: +6/-0
  • Offline Offline
  • Posts: 44
    • View Profile
Proposal: Create unit tests for two classes
« 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
  • Each tested class have a line code coverage higher than 80%
  • Written in Java and using JUnit
  • Fast, independent, no access to external resources
Logged

Jean-Luc

  • Core Dev
  • Hero Member
  • *****
  • Karma: +816/-81
  • Offline Offline
  • Posts: 1610
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #1 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.

Logged
GPG key fingerprint: 263A 9EB0 29CF C77A 3D06  FD13 811D 6940 E1E4 240C
NXT-X4LF-9A4G-WN9Z-2R322

jonny

  • Jr. Member
  • **
  • Karma: +6/-0
  • Offline Offline
  • Posts: 44
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #2 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.
Logged

wesley

  • Ex-Staff Member
  • Hero Member
  • *****
  • Karma: +204/-3
  • Offline Offline
  • Posts: 1159
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #3 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.
Logged

LocoMB

  • Hero Member
  • *****
  • Karma: +101/-37
  • Offline Offline
  • Posts: 751
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #4 on: July 28, 2014, 06:53:58 am »


I vote in favour of the proposal.
Logged
TOX
90E54E5B5213290EE616D425CADC473038CFABFA53C913271AA8559D1937DC4AF3A354A9E4E5

antanst

  • Full Member
  • ***
  • Karma: +36/-0
  • Offline Offline
  • Posts: 216
    • View Profile
    • nxtblocks.info
Re: Proposal: Create unit tests for two classes
« Reply #5 on: July 28, 2014, 07:10:05 am »

You have my approval as well, go ahead and get your feet wet jonny.
Logged

jonny

  • Jr. Member
  • **
  • Karma: +6/-0
  • Offline Offline
  • Posts: 44
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #6 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.
Logged

jonny

  • Jr. Member
  • **
  • Karma: +6/-0
  • Offline Offline
  • Posts: 44
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #7 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

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

LocoMB

  • Hero Member
  • *****
  • Karma: +101/-37
  • Offline Offline
  • Posts: 751
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #8 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

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?
Logged
TOX
90E54E5B5213290EE616D425CADC473038CFABFA53C913271AA8559D1937DC4AF3A354A9E4E5

jonny

  • Jr. Member
  • **
  • Karma: +6/-0
  • Offline Offline
  • Posts: 44
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #9 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
Logged

Jean-Luc

  • Core Dev
  • Hero Member
  • *****
  • Karma: +816/-81
  • Offline Offline
  • Posts: 1610
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #10 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.
Logged
GPG key fingerprint: 263A 9EB0 29CF C77A 3D06  FD13 811D 6940 E1E4 240C
NXT-X4LF-9A4G-WN9Z-2R322

jonny

  • Jr. Member
  • **
  • Karma: +6/-0
  • Offline Offline
  • Posts: 44
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #11 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.
Logged

jonny

  • Jr. Member
  • **
  • Karma: +6/-0
  • Offline Offline
  • Posts: 44
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #12 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
Logged

Jean-Luc

  • Core Dev
  • Hero Member
  • *****
  • Karma: +816/-81
  • Offline Offline
  • Posts: 1610
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #13 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.
Logged
GPG key fingerprint: 263A 9EB0 29CF C77A 3D06  FD13 811D 6940 E1E4 240C
NXT-X4LF-9A4G-WN9Z-2R322

jonny

  • Jr. Member
  • **
  • Karma: +6/-0
  • Offline Offline
  • Posts: 44
    • View Profile
Re: Proposal: Create unit tests for two classes
« Reply #14 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.
Logged
 

elective-stereophonic
elective-stereophonic
assembly
assembly