jackson-core icon indicating copy to clipboard operation
jackson-core copied to clipboard

Complete Junit5 migration

Open timo-a opened this issue 1 year ago • 1 comments

This PR will complete the migration to Junit5.

I've used an OpenRewrite recipe to automate the migration. I applied some manual fixes were necessary. I had to disable 5 tests, but those were simply ignored under junit4, so the migration didn't break them, they just became apparent. They are

AsyncNonStdParsingTest.testNonStandarBackslashQuotingForValues(int)
AsyncScopeMatchingTest.testEOFInName(int)
AsyncScopeMatchingTest.testMisssingColon(int)
AsyncScopeMatchingTest.testUnclosedArray(int)
AsyncScopeMatchingTest.testUnclosedObject(int)

they all take a parameter, but there is no mechanism to supply the parameter. With JUnit 4 this lead to them being skipped, but in JUnit 5 they are not skipped and fail. I've added the @Disabled annotation to them in 8dc98458. I don't know how to fix those, If you want them please fix them yourselves. I've replaced BaseTest with JUnitTestBase (and renamed that to TestBase) In the end, I applied some cleanup recipes.

timo-a avatar Mar 09 '24 23:03 timo-a

First of all, thank you for submitting this!

Second: I hope it's not difficult to do, but it would be really nice if this could be split into smaller chunks, for one reason: merging 2.17 -> master (3.0) requires manual changes when I do that, so that's rather big task for all remaining test classes. That's why we do conversion on jackson-databind in smaller batches.

cowtowncoder avatar Mar 10 '24 20:03 cowtowncoder

I see, I've opened https://github.com/FasterXML/jackson-core/issues/1239 to coordinate the batches and opened a new PR https://github.com/FasterXML/jackson-core/pull/1238 with just a subset of tests migrated. The new PR has 21 files, please let me know in case that is still too many, I don't have a good intuition on the batch size.

timo-a avatar Mar 16 '24 23:03 timo-a

That size looks reasonable -- I'll see how easy it is to merge to master (3.0).

I also changed default branch to 2.18; merging via 2.17 is fine as well as 2.18, no strong opinion there.

Note, too, that there is relatively new src/test/java/com/fasterxml/jackson/core/JUnit5TestBase.java (JUnit 5 referencing) to replace existing usage of BaseTest (junit4).

cowtowncoder avatar Mar 21 '24 01:03 cowtowncoder