RFR: JDK-8315569: Tests for the contract of SkinBase.layoutChildren(..) [v2]

[Marius Hanl]

On Thu, 7 Sep 2023 22:04:46 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Marius Hanl has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   JDK-8315569: Set a min size
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlContractTest.java line 44:
> 
>> 42: import static org.junit.jupiter.api.Assertions.assertTrue;
>> 43: 
>> 44: class ControlContractTest {
> 
> Do you plan to add more tests to this class?  The name does not specify which contract is being tested.

Do you a better idea? I guess it would make sense to rename it to something more generic. Maybe ControlLayoutTest?

> modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlContractTest.java line 46:
> 
>> 44: class ControlContractTest {
>> 45: 
>> 46:     private ControlStub control;
> 
> Do you think it's worth to run this test against all of the existing Controls?

Since no `Control` is overriding the `layoutChildren` method, I don't think it will make any difference as of now

> modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlContractTest.java line 67:
> 
>> 65:      */
>> 66:     @ParameterizedTest
>> 67:     @ValueSource(doubles = { 1.0, 1.25, 1.5, 1.75, 2.0 })
> 
> Could you please add 2.25 (I can see this one with the secondary monitor attached to my win11 box)

sure.

> modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlContractTest.java line 101:
> 
>> 99: 
>> 100:                 double expectedWidth = control.snapSizeX(control.getWidth()) - control.snappedLeftInset() - control.snappedRightInset();
>> 101:                 assertEquals(expectedWidth, w);
> 
> since we are performing floating point operations, we might accumulate small error.  
> So this and 3 subsequent assertEquals() would need an EPSILON = 0.0000001 (an arbitrary value)

Will have a look, but it worked without so I did not bothered adding an epsilon to the tests

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1322547208
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1322546539
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1322548432
PR Review Comment: https://git.openjdk.org/jfx/pull/1229#discussion_r1322550303