RFR: JDK-8315951: Open source several Swing HTMLEditorKit related tests
Alexey Ivanov
aivanov at openjdk.org
Mon Sep 18 11:09:48 UTC 2023
On Thu, 14 Sep 2023 18:45:34 GMT, Harshitha Onkar <honkar at openjdk.org> wrote:
> Following tests are open sourced as part of this PR.
>
> 1. java/awt/event/PaintEvent/RepaintTest/RepaintTest.java
> 2. javax/swing/text/html/HTMLEditorKit/4214848/bug4214848.java
> 3. javax/swing/text/html/HTMLEditorKit/4230197/bug4230197.java
> 4. javax/swing/text/html/HTMLEditorKit/4238223/bug4238223.java
Changes requested by aivanov (Reviewer).
test/jdk/java/awt/event/PaintEvent/RepaintTest.java line 44:
> 42: // invoking setBounds, and the other that invokes repaint directly on
> 43: // the Frame to repaint a Component. The Component displays an integer
> 44: // that increments everytime paint is invoked on it.
This comment seems confusing because no buttons are created in the test.
test/jdk/java/awt/event/PaintEvent/RepaintTest.java line 63:
> 61:
> 62: int count = counter.getCount();
> 63: robot.delay(300);
Does this delay affect anything? I think it can be safely removed.
test/jdk/java/awt/event/PaintEvent/RepaintTest.java line 65:
> 63: robot.delay(300);
> 64:
> 65: EventQueue.invokeAndWait(() -> panel.repaint());
Use method reference too?
Suggestion:
EventQueue.invokeAndWait(panel::repaint);
test/jdk/java/awt/event/PaintEvent/RepaintTest.java line 71:
> 69: if (counter.getCount() == count) {
> 70: throw new RuntimeException("Failed");
> 71: }
This is not thread-safe. The `paintCount` in `IncrementComponent` needs to be volatile. Better yet, use `AtomicInteger` and its [`incrementAndGet`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/concurrent/atomic/AtomicInteger.html#incrementAndGet()) or [`getAndIncrement`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/concurrent/atomic/AtomicInteger.html#getAndIncrement()) as well as [`get`](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/concurrent/atomic/AtomicInteger.html#get()), it will make the test code thread-safe.
test/jdk/javax/swing/text/html/HTMLEditorKit/bug4214848.java line 40:
> 38: StringWriter sw = new StringWriter();
> 39: String test1="<HTML><BODY></BODY></HTML>";
> 40: HTMLEditorKit c= new HTMLEditorKit();
Suggestion:
String test1 = "<HTML><BODY></BODY></HTML>";
HTMLEditorKit c = new HTMLEditorKit();
Using `c` for the editor kit doesn't seem a good idea, `k` or `kit` would be better.
`test1` could be just `test`, there are no other test strings.
test/jdk/javax/swing/text/html/HTMLEditorKit/bug4214848.java line 44:
> 42: c.read(new StringReader(test1), doc, 0); // prepare test document
> 43: c.write(sw, doc, 0, 10);
> 44: String au=sw.toString().toLowerCase();
Suggestion:
String au = sw.toString().toLowerCase();
I'd also rename the variable to `out` which is more meaningful.
test/jdk/javax/swing/text/html/HTMLEditorKit/bug4230197.java line 41:
> 39: StringWriter sw = new StringWriter();
> 40: HTMLDocument doc = (HTMLDocument) kit.createDefaultDocument();
> 41: kit.insertHTML(doc, 0, "<sub>0</sub>", 0, 0, HTML.Tag.SUB);
Suggestion:
kit.insertHTML(doc, doc.getLenth(), "<sub>0</sub>", 0, 0, HTML.Tag.SUB);
for consistency… It shouldn't change the logic, yet it's better to verify whether this update still reproduces the original problem.
test/jdk/javax/swing/text/html/HTMLEditorKit/bug4230197.java line 48:
> 46: kit.write(sw, doc, 0, doc.getLength());
> 47:
> 48: String au = sw.toString().toLowerCase();
Suggestion:
String out = sw.toString().toLowerCase();
test/jdk/javax/swing/text/html/HTMLEditorKit/bug4230197.java line 51:
> 49: if ((!au.contains("<sub>0</sub>")) || (!au.contains("<sup>0</sup>"))
> 50: || (!au.contains("<code>0</code>")) || (!au.contains("<b>0</b>"))
> 51: || (!au.contains("<i>0</i>"))) {
I'd put each condition on its own line for easier parsing:
Suggestion:
if ((!au.contains("<sub>0</sub>"))
|| (!au.contains("<sup>0</sup>"))
|| (!au.contains("<code>0</code>"))
|| (!au.contains("<b>0</b>"))
|| (!au.contains("<i>0</i>"))) {
test/jdk/javax/swing/text/html/HTMLEditorKit/bug4238223.java line 53:
> 51: public void handleComment(char[] data, int pos) {
> 52: if (! (new String(data)).equals(commentData) ||
> 53: pos != commentIndex) {
Suggestion:
if (!(new String(data)).equals(commentData)
|| pos != commentIndex) {
There are no spaces after the `!` operator in the following `if` statements. I would also suggest wrapping before the operator.
test/jdk/javax/swing/text/html/HTMLEditorKit/bug4238223.java line 65:
> 63: public void handleEndTag(HTML.Tag tag, int pos) {
> 64: if (! tag.equals(endTags[endTagIndex]) ||
> 65: pos != endTagPositions[endTagIndex]) {
Suggestion:
if (!tag.equals(endTags[endTagIndex])
|| pos != endTagPositions[endTagIndex]) {
No space after `!`, wrap before the operator.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15751#pullrequestreview-1630751026
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328542244
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328539646
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328540293
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328538649
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328547079
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328550046
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328554453
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328555225
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328556251
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328561252
PR Review Comment: https://git.openjdk.org/jdk/pull/15751#discussion_r1328562325
More information about the client-libs-dev
mailing list