[TRIVIAL] Cleanup the JNI test file
ClosedPublic
Actions

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

Details

Reviewers

deadalnix
jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGING0cc9cba736ea: [TRIVIAL] Cleanup the JNI test file
rABC0cc9cba736ea: [TRIVIAL] Cleanup the JNI test file

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 Passed

Unit

No Test Coverage

Build Status

Buildable 7093
Build 12232: Bitcoin ABC Buildbot (legacy)
Build 12231: arc lint + arc unit

Event Timeline

Fabien created this revision.Aug 9 2019, 10:38

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 9 2019, 10:38

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Diff Testing started.

Fabien added a child revision: D3835: [SECP256K1] JNI tests : remove dependency to obsolete DatatypeConverter.Aug 9 2019, 10:46

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7093: Diff 10695.Aug 9 2019, 10:49

jasonbcox requested changes to this revision.Aug 9 2019, 15:45

jasonbcox added a subscriber: jasonbcox.

jasonbcox added inline comments.

src/secp256k1/src/java/org/bitcoin/NativeSecp256k1Test.java
13	mixed comment types
196	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.Aug 9 2019, 15:45

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

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7114: Diff 10725.Aug 12 2019, 13:02

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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

@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.Aug 13 2019, 21:38

This revision is now accepted and ready to land.Aug 13 2019, 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

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