RFR: 8214699: Node.getPseudoClassStates must return the same instance on every call [v2]

Ambarish Rapte arapte at openjdk.java.net
Tue Jun 23 10:33:27 UTC 2020


On Mon, 22 Jun 2020 12:08:21 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> modules/javafx.graphics/src/test/java/test/javafx/scene/NodeTest.java line 168:
>> 
>>> 167:         assertSame("getPseudoClassStates() should always return the same instance",
>>> 168:                 node.getPseudoClassStates(), node.getPseudoClassStates());
>>> 169:     }
>> 
>> To make this more clear, I recommend to store the expected value in a local variable and then call assert.
>
> would suggest some more tests (overcautious maybe, be have seen horses vomit at unbelievable places ;)
> 
> Test that the returned set
> 
> - is indeed unmodifiable
> - not gc'ed
> - is a wrapper around the backing set
> 
> Test code:
> 
>     @Test(expected=UnsupportedOperationException.class)
>     public void testPseudoClassesUnmodifiable() {
>         Node node = new Rectangle();
>         node.getPseudoClassStates().add(PseudoClass.getPseudoClass("dummy"));
>     }
> 
>     @Test
>     public void testPseudoClassesNotGCed() {
>         Node node = new Rectangle();
>         WeakReference<Set<?>> weakRef = new WeakReference<>(node.getPseudoClassStates());
>         ControlSkinFactory.attemptGC(weakRef);
>         assertNotNull("pseudoClassStates must not be gc'ed", weakRef.get());
>     }
>     
>     // requires additional method in NodeShim
>     @Test
>     public void testUnmodifiablePseudoClassStatesEqualsBackingStates() {
>         Rectangle node = new Rectangle();
>         PseudoClass pseudo = PseudoClass.getPseudoClass("somepseudo");
>         node.pseudoClassStateChanged(pseudo, true);
>         assertEquals(1, node.getPseudoClassStates().size());
>         assertEquals(NodeShim.pseudoClassStates(node).size(), node.getPseudoClassStates().size());
>         assertTrue(NodeShim.pseudoClassStates(node).contains(pseudo));
>         assertTrue(node.getPseudoClassStates().contains(pseudo));
>     }

Corrected the existing test and added these new tests, with minor changes.
ControlSkinFactory is a file from control tests. So have added a copy of `attemptGC()` method in
`test.javafx.scene.shape.TestUtils` class. The class does not seem to be a perfect location for `attemptGC()`. But
wanted to avoid a new util kind off class just for this one method.

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

PR: https://git.openjdk.java.net/jfx/pull/253


More information about the openjfx-dev mailing list