RFR: 8255337: [TestBug] Controls unit tests - ButtonTest and ComboBoxTest - log ClassCastException
Ajit Ghaisas
aghaisas at openjdk.java.net
Tue Oct 27 05:45:20 UTC 2020
On Fri, 23 Oct 2020 15:44:43 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:
>> This is a test fix.
>>
>> Root cause:
>> - For ButtonTest - Incorrect use of MouseEventFirer - which puts Button on to the stage and shows it before firing mouse event
>> - For ComboBoxTest - adding ComboBox to a Stage, not showing the Stage but trying to show the ComboBox
>>
>>
>> Fix :
>> - For ButtonTest - Moved MouseEventFirer usage from test class to the 2 tests that need it
>> - For ComboBoxTest - Started using StageLoader (which adds comboBox to the stage and shows it) instead of separate Scene and Stage creation.
>>
>>
>> I have attached the logs captured before and after this fix to the JBS.
>
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ButtonTest.java line 425:
>
>> 423:
>> 424: mouse.dispose();
>> 425: }
>
> just curious: do we have to dispose the mouseFirer? If so, that pattern isn't safe because it will not happen if the test fails.
Hmm.. very good point. I believe, we should use dispose mechanism whenever available for a readable cleanup. Both classes MouseEventFirer & StageLoader provide dispose method and the user of these classes should use it.
Now, this can be handled in two ways -
1) Keep a reference at test class level - it will be null by default - individual tests will create the object and update the class reference - we can dispose in cleanup method after the test
2) Use Try blocks in tests that use MouseEventFirerer or StageLoader objects and dispose them in finally{} blocks.
We can use (1) if many tests in a test class use MouseEventFirerer or StageLoader objects - for example ComboBoxTest and use approach (2) if very few tests use these classes - for example ButtonTest.
> modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java line 658:
>
>> 656: StageLoader sl = new StageLoader(comboBox);
>> 657:
>> 658:
>
> good catch :) But wouldn't show the stage have the same effect? (It's just me having a personal dislike of stageloader :)
stage.show() has the same effect of fixing the bug. In fact, I fixed it initially with exact this fix.
When I looked at the other tests in ComboBoxTest class, I saw that there is a pattern of creating a Stage, creating a Scene with ComboBox and then adding Scene to the Stage. What was missed was stage.show(). The StageLoader does exactly these steps (including the missed stage.show()).
StageLoader might be a misfit at some of the other places in our tests (I believe that is the reason for your dislike), but I feel using StageLoader is a better fit in ComboBoxTest.
-------------
PR: https://git.openjdk.java.net/jfx/pull/337
More information about the openjfx-dev
mailing list