On Thu, 2 May 2024 23:32:19 GMT, Oliver Kopp <d...@openjdk.org> wrote:
>> The fix looks good, I have few comments about the test. >> 1. The test does not compile without fix, hence it won't fail without fix as >> we are only testing the newly added helper method. >> 2. It is not required to change the error stream, as the test does/need not >> inspect the error output. >> 3. The test method and parameter source method names can be changed a little. >> 4. We should use the Util class for standard setup like initializing JavaFX/ >> shutting it down. >> >> >> I tried above changes to test locally, attaching the file for ease. >> [WinTextRangeProviderTest.zip](https://github.com/openjdk/jfx/files/15186626/WinTextRangeProviderTest.zip) > >> 1. The test does not compile without fix, hence it won't fail without >> fix as we are only testing the newly added helper method. > > Yes, we could not build on existing a11y test. > >> 2. It is not required to change the error stream, as the test does/need >> not inspect the error output. > > Nice! > >> 3. The test method and parameter source method names can be changed a >> little. > > Note that `@MethodSoruce` can be used without any parameter - then the same > name is used. OK, this is not the style of JFX. > > For JUnit5, the `test` prefix is not necessary any more (and can be removed - > see > https://docs.openrewrite.org/recipes/java/testing/cleanup/removetestprefix), > but I think, because of consistency, it is kept. > >> 4. We should use the Util class for standard setup like initializing >> JavaFX/ shutting it down. > > Nice! > >> I tried above changes to test locally, attaching the file for ease. >> [WinTextRangeProviderTest.zip](https://github.com/openjdk/jfx/files/15186626/WinTextRangeProviderTest.zip) > > Thank you. I committed at d03bdd40a3340bd85397f (with you as author, hope > this is in line with the policies?!). @koppor This is now ready for you to `/integrate`. ------------- PR Comment: https://git.openjdk.org/jfx/pull/1442#issuecomment-2100457469
