Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Fri, 24 Sep 2021 20:45:23 GMT, Kevin Rushforth wrote: > As mentioned in JBS, any new third-party libraries require prior third-party > license approval. And we will need to work with you on sponsoring this (as > you can't contribute any third-party code under the OCA). > > Speaking of which, there are more libraries added to > `gradle-verification.xml` than I would expect. Each one will need third-party > approval, if they are required. Since this is for internal use (build / test > only) and not something we redistribute, that makes it easier, although it > still depends on the license for each piece. > > Finally, we have some closed tests that we need to ensure aren't impacted by > this, so that's one more area we need to coordinate. > > We can take a look, but it won't be right away. Can you also start a thread > on openjfx-dev? I'd like to gauge the level of interest in enabling JUnit 5 > for new tests. Github is showing this as a "requested change" but I can't resolve this. @kevinrushforth anything you can do on your end? All the issues in this comment were addresses AFAIK. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Thu, 11 Nov 2021 15:11:32 GMT, Kevin Rushforth wrote:
>> John Hendrikx has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Readd junit declaration in allprojects and set junit version to 4.13.2
>
> build.gradle line 1953:
>
>> 1951: testRuntimeOnly(group: "org.junit.vintage", name:
>> "junit-vintage-engine", version: "5.8.1") {
>> 1952: exclude(group: "org.apiguardian", module:
>> "apiguardian-api")
>> 1953: }
>
> As it turns out, `apiguardian` no longer needs to be blocked, so you can
> restore it if it is easier.
Okay, I'll remove the exclusion on apiguardian then as it would just be
confusing why it was excluded in the first place. Hamcrest was already
included as JUnit 5 wouldn't work without it.
-
PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Sat, 25 Sep 2021 13:55:15 GMT, John Hendrikx wrote: >> I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests >> still work. Also added a single JUnit 5 tests, and confirmed it works. >> >> I've updated the Eclipse project file for the base module only. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Readd junit declaration in allprojects and set junit version to 4.13.2 These are good points regarding test migration. I withdraw the suggestion of test segregation. I would not want to see a wholesale migration of all tests to JUnit5, but I do like the idea of migrating on a test-by-test basis if new tests are being added to an existing test class where using JUnit 5 would be a benefit. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Mon, 15 Nov 2021 16:42:04 GMT, Michael Strauß wrote: > One comment about adding new JUnit 5 tests and migrating existing tests. I > think there could be value in organizing the tests such that all of the JUnit > 5 tests are grouped, rather than mixing tests in the same directory such that > some use JUnit 5 and others use JUnit 4. What do you (or others) think? We > could either do this with a new JUnit 5 source set in each project, or by > using a package naming convention for JUnit 5 tests like we do for robot > tests. Maybe `test5.some.pkg`. This needs more thought. I don't think this is really needed, since the introduction of JUnit 5 I've written numerous tests in projects with the bulk of their test classes still using JUnit 4. In all cases, the new tests were just mixed with the old tests. The mixing of the tests has so far caused no confusion, and in these projects the teams involved also didn't feel any need to prioritize migrating all tests to JUnit 5. Conversion to JUnit 5 usually happens on a test by test basis when other changes are made to the test -- existing code is rewritten as needed, usually limited to introducing `assertThrows` to replace `@Test(expected = Throwable.class)` and updating all static asserts to use the `jupiter` package hierarchy instead of the `org.junit` one (just import changes). Usually this is done in a separate commit (a "chore" commit) with another commit containing the actual change and new test cases. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Thu, 11 Nov 2021 15:18:25 GMT, Kevin Rushforth wrote: > I left a few comments on the dependencies. Will review / test the PR later. > > One comment about adding new JUnit 5 tests and migrating existing tests. I > think there could be value in organizing the tests such that all of the JUnit > 5 tests are grouped, rather than mixing tests in the same directory such that > some use JUnit 5 and others use JUnit 4. What do you (or others) think? We > could either do this with a new JUnit 5 source set in each project, or by > using a package naming convention for JUnit 5 tests like we do for robot > tests. Maybe `test5.some.pkg`. This needs more thought. I don't like the idea of separating source sets or packages just based on the version of our unit test framework. In my opinion, sources should be organized by function, and not based on a technicality. A very common case we'll encounter will be adding a new unit test to an existing test class. What's our guidance in this case? If we continue to use JUnit4 for new unit tests, that means we'll effectively persist this version of the framework forever in the test sources. Another option would be to migrate the entire test class once it's updated with new code. However, that would be a significant amount of work required from the author of a new unit test. A third option would be to actually migrate all unit test classes as a series of refactoring PRs. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Fri, 24 Sep 2021 21:17:14 GMT, John Hendrikx wrote: >> I will try block this and get back to you. > > Hamcrest could be blocked, tests still run. As it turns out, `hamcrest` does not need to be blocked, so you can restore it if it is easier. I'll leave it up to you. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Sat, 25 Sep 2021 13:55:15 GMT, John Hendrikx wrote:
>> I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests
>> still work. Also added a single JUnit 5 tests, and confirmed it works.
>>
>> I've updated the Eclipse project file for the base module only.
>
> John Hendrikx has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Readd junit declaration in allprojects and set junit version to 4.13.2
I left a few comments on the dependencies. Will review / test the PR later.
One comment about adding new JUnit 5 tests and migrating existing tests. I
think there could be value in organizing the tests such that all of the JUnit 5
tests are grouped, rather than mixing tests in the same directory such that
some use JUnit 5 and others use JUnit 4. What do you (or others) think? We
could either do this with a new JUnit 5 source set in each project, or by using
a package naming convention for JUnit 5 tests like we do for robot tests. Maybe
`test5.some.pkg`. This needs more thought.
build.gradle line 1953:
> 1951: testRuntimeOnly(group: "org.junit.vintage", name:
> "junit-vintage-engine", version: "5.8.1") {
> 1952: exclude(group: "org.apiguardian", module: "apiguardian-api")
> 1953: }
As it turns out, `apiguardian` no longer needs to be blocked, so you can
restore it if it is easier.
-
PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Sat, 25 Sep 2021 13:55:15 GMT, John Hendrikx wrote: >> I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests >> still work. Also added a single JUnit 5 tests, and confirmed it works. >> >> I've updated the Eclipse project file for the base module only. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Readd junit declaration in allprojects and set junit version to 4.13.2 All tasks needed to get third-party approval are now completed. You can move this PR back to ready and the review can proceed. - PR: https://git.openjdk.java.net/jfx/pull/633
Re: RFR: 8274274: Update JUnit to version 5.8.1 [v5]
On Sat, 25 Sep 2021 13:55:15 GMT, John Hendrikx wrote: >> I've added JUnit 5 as a test dependency and made sure that the JUnit 4 tests >> still work. Also added a single JUnit 5 tests, and confirmed it works. >> >> I've updated the Eclipse project file for the base module only. > > John Hendrikx has updated the pull request incrementally with one additional > commit since the last revision: > > Readd junit declaration in allprojects and set junit version to 4.13.2 I was on holidays, I've updated the title. - PR: https://git.openjdk.java.net/jfx/pull/633
