On Thu, 19 Sep 2024 21:04:27 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> should we set a minimum of two reviewers?
I agree, there should be another pair of eyes looking at this. Even though it's
quite repetitive, it is quite a big change,
> modules/javafx.graphics/src/test/java/test/javafx/concurrent/ServiceTestBase.java
> line 65:
>
>> 63: // NOTE: This should be reverted once parametrized class tests are
>> added to JUnit5
>> 64: // For now, tests call this manually
>> 65: // @BeforeEach
>
> I wonder if we should log a JBS ticket to fix parameterized tests by
> reverting `@BeforeEach`, and then simply refer to it?
> This way, once junit supports that, we can easily grep the code.
>
> (I've used a different comment for same purpose)
>
> What do you think?
As mentioned in another comment, it's unlikely we'll spend more time on this
later on. I will remove all the NOTE comments and commented out imports and
such.
> modules/javafx.graphics/src/test/java/test/javafx/stage/WindowTest.java line
> 222:
>
>> 220:
>> 221: @Test
>> 222: public void testSizeToSceneBeforeEachShowing() {
>
> this looks like unrelated change:
> - why was the name changed?
> - why change this method and not testSizeToSceneAfterShowing?
Oops, it is unintentional yeah.
Most of this patch was done with multi-caret editing to tackle multiple spots
affected in the same way:
- Mark affected spot
- Ask my IDE to find and put a caret on all other occurrences of selected text
- Edit them all at once
Being in the rhythm of changing imports and `@Before` to `@BeforeEach` I
probably unintentionally selected and changed this occurrence of the word
"Before". I'll revert this.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1566#issuecomment-2363084472
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1768167064
PR Review Comment: https://git.openjdk.org/jfx/pull/1566#discussion_r1768164494
