Page MenuHomePhabricator

[TRIVIAL] Cleanup the JNI test file
AcceptedPublic

Authored by Fabien on Fri, Aug 9, 10:38.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

Cleanup the NativeSecp256k1Test.java file:

  • Move comments on their own lines;
  • Add period at the end of sentences;
  • Remove extra whitespaces in parenthesis to make the code consistent;
  • Add/Remove some newlines where needed;
  • Remove commented out debug code;
  • Replace tabs with spaces;
  • ...
Test Plan
./contrib/teamcity/build-secp256k1.sh

Diff Detail

Repository
rABC Bitcoin ABC
Branch
secp256k1_cleanup_java_test
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7114
Build 12274: Bitcoin ABC Teamcity Staging
Build 12273: arc lint + arc unit

Event Timeline

Fabien created this revision.Fri, Aug 9, 10:38
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Aug 9, 10:38
jasonbcox requested changes to this revision.Fri, Aug 9, 15:45
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java
13 ↗(On Diff #10695)

mixed comment types

196 ↗(On Diff #10695)

Kind of weird to append a period on the end of a link. maybe do this instead?

* It tests the following test vectors:
* https://github.com/sipa/bips/blob/bip-schnorr/bip-schnorr/test-vectors.csv
This revision now requires changes to proceed.Fri, Aug 9, 15:45
Fabien updated this revision to Diff 10725.Mon, Aug 12, 12:53

Update as per comment.
Add a newline between the TODO and the javadoc comments for better readability.

Are you really confident you want to take ownership of this?

Fabien added a comment.Tue, Aug 13, 06:48

@deadalnix It's already the case, see D2681 or D2755. Also I have D3835 that will take ownership even more, but make your life easier for testing with buster.

jasonbcox accepted this revision.Tue, Aug 13, 21:38
This revision is now accepted and ready to land.Tue, Aug 13, 21:38
In D3834#90773, @Fabien wrote:

@deadalnix It's already the case, see D2681 or D2755. Also I have D3835 that will take ownership even more, but make your life easier for testing with buster.

D3835 clearly have happened one way or another already: https://github.com/bitcoin-core/secp256k1/blob/master/src/java/org/bitcoin/NativeSecp256k1Test.java

Fabien added a comment.Wed, Aug 14, 07:39

@deadalnix Absolutely, sepc256k1 relies on BaseEncoding from the guava dependency which was removed from our codebase by D2681, hence the custom function in D3835.