-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix build on jdk11 and jdk12. remove powermock and jmockit #3081
Conversation
ignore 2 or 3 tests that are not possible to convert now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job. I have only one small remark.
@Before | ||
public void setup() throws IOException { | ||
|
||
tsm = mock(TradeStatisticsManager.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid acronyms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that's what it was called before, will change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
@@ -51,7 +51,7 @@ | |||
@EqualsAndHashCode | |||
@Getter | |||
@Slf4j | |||
public final class OfferPayload implements ProtectedStoragePayload, ExpirablePayload, RequiresOwnerIsOnlinePayload { | |||
public class OfferPayload implements ProtectedStoragePayload, ExpirablePayload, RequiresOwnerIsOnlinePayload { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should stay final for security reasons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is final (not in this commit but later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah saw it now... thanks.
|
||
import org.bitcoinj.core.Coin; | ||
|
||
public class MakerFeeMaker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MakerFeeMaker sounds a bit weird. Maybe MakerFeeProvider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
on jdk 12 the app still throws a NPE at startup (but afterwards it seems to work), but runs fine on jdk11.
build and test suite works on jdk10,11,12, inside IDEA or with
./gradlew clean build
jdk 10 is probably still needed for binaries. but contributors can now use any jdk 10+ that they like
had to disable a very small amount of tests (i think 3) that were not so easy to convert.