On Fri, 4 Apr 2025 11:30:04 GMT, Lukasz Kostyra <lkost...@openjdk.org> wrote:
> This change rewrites `Popup_parentWindow_Test` after a-long-time-ago-done
> changes to Popup public API.
>
> Popup used to have an `owner` and `parentWindow` properties. I couldn't track
> how far back that change happened so I can't fully say what these Properties
> actually were, but to my knowledge both properties were replaced by
> `ownerNode` and `ownerWindow` Properties respectively. New Properties are
> read-only and are set while calling respective `Popup.show()` -
> `Popup.show(Node, ...)` sets both Properties, while `Popup.show(Window, ...)`
> sets only `ownerWindow` Property.
>
> A lot of original test scenarios were cut out because with current Popup API
> they make little to no sense. Original `Popup_parentWindow_Test` extended
> `PropertiesTestBase` and ran the regular Property access checks. Those are
> parametrized and split into four test methods:
> - `testGetBean` - confidence check for whether we can get the Property bean,
> unnecessary since new tests access the Properties directly
> - `testGetName` - Properties in `PropertiesTestBase` are referred to by
> String-provided name and Reflection, so this was a confidence check whether
> those Properties actually exist in the object - unnecessary, since we access
> the Properties directly
> - `testBinding` - I deemed those not doable, since the Properties we try to
> access are read-only and cannot be bound to
> - `testBasicAccess` - This is the only test I found that was workable into a
> reimplementation, and so it is manually reimplemented as `Popup_owner_Test`
>
> Moreover, because of lack of `Popup.setParent()` API I also deemed more
> complex test scenarios (cases referring to TestScene's `_window` Property)
> unnecessary. Since there is no `Popup.show(Scene, ...)` those cases were also
> eliminated. This leaves current test pool into three separate parameters for
> one, similar test case.
lgtm, with some very minor comments.
modules/javafx.graphics/src/test/java/test/javafx/stage/Popup_owner_Test.java
line 55:
> 53: * Instead we rely on checking this manually.
> 54: */
> 55: public final class Popup_owner_Test {
minor: maybe `Popup_Owner_Test`?
modules/javafx.graphics/src/test/java/test/javafx/stage/Popup_owner_Test.java
line 245:
> 243:
> 244: public void assertCalled() {
> 245: if (counter != expected) {
minor: this can probably be replaced by `assertEquals()`
modules/javafx.graphics/src/test/java/test/javafx/stage/Popup_owner_Test.java
line 251:
> 249:
> 250: public void assertNotCalled() {
> 251: if (counter != 0) {
minor: `assertTrue()` here?
-------------
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1763#pullrequestreview-2743813240
PR Review Comment: https://git.openjdk.org/jfx/pull/1763#discussion_r2029269310
PR Review Comment: https://git.openjdk.org/jfx/pull/1763#discussion_r2029263496
PR Review Comment: https://git.openjdk.org/jfx/pull/1763#discussion_r2029264546
