On Mon, 7 Apr 2025 08:09:32 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. > > Lukasz Kostyra has updated the pull request incrementally with one additional > commit since the last revision: > > Replace fail() calls with assertions Test looks good. We can have another bug to address the naming convention to change other similar classes too. Suggesting one minor change. modules/javafx.graphics/src/test/java/test/javafx/stage/Popup_owner_Test.java line 2: > 1: /* > 2: * Copyright (c) 2011, 2025, Oracle and/or its affiliates. All rights > reserved. Either we should use `git mv <oldFile> <newFile>` here to retain the file commit history. or As the test changed a lot from before, we could choose not to do `git mv`, in the case the copyright in new file should be only 2025 I would recommend to `git mv` as a general good practice. ------------- Changes requested by arapte (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1763#pullrequestreview-2752441223 PR Review Comment: https://git.openjdk.org/jfx/pull/1763#discussion_r2034704135
