jsonschema2pojo icon indicating copy to clipboard operation
jsonschema2pojo copied to clipboard

Junit 5 and build on Java 9+ (including Java 17)

Open eirnym opened this issue 3 years ago • 6 comments
trafficstars

Introduce Junit5 and ability to build on Java 17.

General JUnit5 sidenote:

  • Roboleptic has no support for JUnit5 and test is needed to be replaced with another one if you want to compile this class with Android Build Tools

Java 9+ sidenotes:

  • For Java 9+ minimal version of targetJava in tests is 1.7, not 1.6 as per compiler support
  • Due to logic in AnnotationHelper some tests will always fail on Java 9+ even Target Java Version is 1.7 (thus these tests are disabled)

eirnym avatar Dec 22 '21 13:12 eirnym

This is great @eirnym.

I also went down a bit of a rabbit-hole with robolectric in the past when attempting to upgrade to a newer version. Based on a little research it might be possible to keep these tests running but using junit-vintage-engine somehow. It is nice to test the Parcelable behaviour as it would be very easy to break it. There may also be a more modern or simpler way to do this that doesn't use robolectric.

I'd like to merge this soon since the changes are so far-reaching. I do want to release 1.1.2 first though as we have a few changes queued up. We can create some new tickets to track remaining tasks.

joelittlejohn avatar Dec 24 '21 12:12 joelittlejohn

Would it have had been a bad idea to have separate PR's for Junit 5 and compilation support for Java17 ? It's rather hard to understand which changes are related to "Junit 5" and which not

unkish avatar Dec 24 '21 12:12 unkish

@unkish Yes, I definitely agree. Best to keep PRs small and focused, and avoid one change running into another. It makes the PR a lot easier to review. I think there are also various edits here that don't strictly relate to the upgrade (of JUnit or Java version) and best to avoid including all these things in one PR. It's often tempting to fix other things you see, but better to keep these separate.

Picking this apart is probably difficult now, and there's a lot of valuable work done here that we shouldn't discard, but this is something to bear in mind for future PRs.

joelittlejohn avatar Dec 24 '21 13:12 joelittlejohn

Actually, the only difference is the few java version checks in tests you can find by usage of Compiler.getJavaVersion method to apply some quirks. Thus I'm fine to separate it out as 2 dependant commits.

eirnym avatar Dec 24 '21 15:12 eirnym

@unkish @joelittlejohn this wasn't about fixing underlying issues why I had to introduce a few quirks.

The main problem with java 9+ would be fixing how Generated annotation is applied. And this is definitely not a part even of this commit.

eirnym avatar Dec 24 '21 16:12 eirnym

@joelittlejohn I'd personally check if the output matches the expected. This part of API and it's behaviour hasn't been changed for years and I don't suppose it ever will

eirnym avatar Dec 24 '21 16:12 eirnym

Closing this as it is out of date, and we'll address these things individually. Thanks for raising though and showing the way forward in these areas.

joelittlejohn avatar Jul 08 '23 21:07 joelittlejohn