search

FasterXML / jackson-core

Core part of Jackson that defines Streaming API as well as basic shared abstractions
Apache License 2.0
2.27k stars 793 forks source link

Complete Junit5 migration #1237

Closed timo-a closed 7 months ago

timo-a commented 7 months ago

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.

cowtowncoder commented 7 months ago

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.

timo-a commented 7 months ago

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.

cowtowncoder commented 7 months ago

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).