pokkst/bitcoincashj

Improve Code Quality

kriptolernen opened this issue · 5 comments

kriptolernen commented

There should not be compilation errors on the git repository. With this code I get "The method isDifficultyTransitionPoint(StoredBlock, NetworkParameters) in the type AbstractBitcoinNetParams is not applicable for the arguments (int)"

All unit tests should run in under a second (that is; all unit tests combined should run in 1 second). Integration Tests should be distinguished from Unit Tests.

No test should fail.

pokkst commented

I hope you know I didn't write those tests, but I fixed those three calls in the asert branch.

pokkst commented

Also: there is no "standard" that unit tests should run in under a second. It entirely depends on what you are testing.

kriptolernen commented

Also: there is no "standard" that unit tests should run in under a second. It entirely depends on what you are testing.

I think we have different ideas about what Unit Tests are. If a test takes more than a second, then it's not an Unit Test, it's probably an Integration Test. Unit Tests should be run after every little change to make sure that the change didn't cause any regression. That's why you need them to run in less than a second.

In fact, you have an example of good written Unit Tests in the source code of the Project you forked from.

I send you a couple of links if you want to get some info abut it:

https://stackoverflow.com/a/10513

https://docs.microsoft.com/en-us/dotnet/core/testing/unit-testing-best-practices

https://www.artima.com/weblogs/viewpost.jsp?thread=126923

This last one explains what an Unit Test is not in the following points:

A test is not a unit test if:
It talks to the database
It communicates across the network
It touches the file system
It can't run at the same time as any of your other unit tests
You have to do special things to your environment (such as editing config files) to run it.

pokkst commented

The unit tests you are criticizing right now were written during the bitcoinj period, before I forked. I've barely touched the tests since forking.

I suggest you take it up with the bitcoinj team then, and not me.

Have a good day.

pokkst commented

I even stated this in my original comment, but you just so happen to conveniently ignore that one.