On Tue, 16 Jul 2024 18:59:00 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> > The new unit test looks like it covers all the cases. As with PR #1505, > > this PR mixes JUnit 4 and JUnit 5, which we really don't want to do in the > > same test class. Consider converting the entire class to JUnit 5, perhaps > > in #1505. > > On closer look, it's probably best to not do that as part of #1505 since that > PR only adds a couple test methods to an existing class with many preexisting > tests. Similarly, this PR only adds one new test method. > > So I think the best options are: > > 1. File a new test bug and PR that converts this class to JUnit 5, that new > bug would block both of the other bugs > 2. Stick with JUnit 4 for both of the existing PRs > > Comments? Perhaps: 3. Put the JUnit 5 code in a separate test? About mixing asserts; I meant to upgrade all of them in this PR, but I missed a few. Mixing asserts however is technically not an issue as both frameworks will work with each others asserts (ie. JUnit 4 can use JUnit 5 asserts, or even asserts from a 3rd party library like AssertJ). What you can't mix is the annotations. You must use all JUnit 4 or JUnit 5 annotations (and this I did do correctly). I would personally prefer not to write more "old" JUnit 4 code than necessary, especially for parameterized cases or testing exceptions (JUnit 4 just is showing its age there), so if full conversion to JUnit 5 is not an option, I'll file a ticket to first convert it. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1503#issuecomment-2231673097
