On Mon, 16 Sep 2024 15:23:48 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/AccordionBehaviorTest.java
>> line 39:
>>
>>> 37:
>>> 38: @Test
>>> 39: public void focusGainedIsCaughtByBehavior() {
>>
>> Wait, is this an empty test? Why?
>
> Intentionally did not want to change the number of tests, which should be
> double checked during the review. Leaving as is for now.
Agree.
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/LabelSkinCreationTest.java
>> line 98:
>>
>>> 96: }
>>> 97:
>>> 98: public record Parameter(
>>
>> Minor: Can also imagine a name like `LabelParameter` or `LabelConfig`
>
> rather, this record should be declared private
Also that is a good idea.
>> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/QueryAccessibleAttributeTest.java
>> line 50:
>>
>>> 48: // @BeforeEach
>>> 49: // junit5 does not support parameterized class-level tests yet
>>> 50: public void setup(Class<Node> nodeClass) {
>>
>> Minor: Since `Control` is used above, should also be used here and below as
>> Generic.
>
> hmm, the code is technically correct since Control extends Node.
yes, just a minor nitpick since relaxing the bounds is not really needed
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1764150535
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1764149338
PR Review Comment: https://git.openjdk.org/jfx/pull/1561#discussion_r1764146934
