On Fri, 15 Sep 2023 19:35:15 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:
> Following tests are open sourced as part of this PR.
>
> 1. test/jdk/javax/swing/text/html/CSS/bug4174871.java
> 2. test/jdk/javax/swing/text/html/CSS/bug4174874.java
> 3. test/jdk/javax/swing/text/html/CSS/bug4284162.java
> 4. test/jdk/javax/swing/text/html/CSS/bug4764897.java
> 5. test/jdk/javax/swing/text/html/HTMLDocument/bug4209280.java
I wonder if any these tests could be made headless. I think there's a potential
to do so. Yet it's out of scope of migration, I guess.
test/jdk/javax/swing/text/html/CSS/bug4174871.java line 29:
> 27: import javax.swing.text.View;
> 28: import java.awt.Robot;
> 29: import java.awt.Shape;
Did we agree on the order of imports, which one goes first `java.*` packages or
`javax.*` packages. Does it depend on the test?
test/jdk/javax/swing/text/html/CSS/bug4174871.java line 41:
> 39: private static JFrame frame;
> 40: private static JTextPane pane;
> 41: private static volatile boolean passed = false;
Explicitly assigning the default value is redundant.
Moreover, you always assign a value in the `testUI` method.
This comment also applies to the tests below.
test/jdk/javax/swing/text/html/CSS/bug4174871.java line 53:
> 51: SwingUtilities.invokeAndWait(bug4174871::testUI);
> 52: robot.waitForIdle();
> 53: robot.delay(200);
After you've performed all the actions, waiting doesn't seem to affect
anything, and therefore it could be removed.
This comment also applies to the tests below.
test/jdk/javax/swing/text/html/CSS/bug4284162.java line 59:
> 57: if (!passed) {
> 58: throw new RuntimeException("Test failed!!" +
> 59: " Borders of HTML table not rendered correctly");
The error message seems to come from the previous test above.
test/jdk/javax/swing/text/html/CSS/bug4284162.java line 95:
> 93: AttributeSet attrs = v.getAttributes();
> 94: Object attr = attrs.getAttribute(StyleConstants.FirstLineIndent);
> 95: passed = (attr.toString().contains("-"));
`startsWith`?
test/jdk/javax/swing/text/html/CSS/bug4764897.java line 58:
> 56: if (!passed) {
> 57: throw new RuntimeException("Test failed!!" +
> 58: " Borders of HTML table not rendered correctly");
The error message seems to be copied from a previous test.
test/jdk/javax/swing/text/html/CSS/bug4764897.java line 99:
> 97:
> 98: if (viewName.endsWith("CellView")) {
> 99: cellsWidth = r.getBounds().x + r.getBounds().width;
This looks weird to me: why does `cellWidth` include the `x` coordinate in its
width? Perhaps, the variable is named incorrectly.
It looks we care about the last cell in the table only, and it is to be
positioned so that it fits inside the table. In other words, the last cell is
within table bounds, that's why `x + width` is used.
test/jdk/javax/swing/text/html/HTMLDocument/bug4209280.java line 39:
> 37: public static void main(String[] args) throws Exception {
> 38: SwingUtilities.invokeAndWait(() -> {
> 39: JFrame jFrame = new JFrame("Unknown HTML tag Test");
I prefer omitting the `j-` prefix from variable name. Frames in the tests above
were named simply `frame`.
test/jdk/javax/swing/text/html/HTMLDocument/bug4209280.java line 41:
> 39: JFrame jFrame = new JFrame("Unknown HTML tag Test");
> 40: String html = "<html><bold>Foo</bold></html>";
> 41: JLabel label = new JLabel(html);
I wonder if the test could be headless. I presume the exception is thrown when
HTML is parsed. Does it happen in constructor? Does it happen when the label is
added to a container?
Is it enough to create the label?
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15769#pullrequestreview-1630692732
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328516488
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328504280
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328505337
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328514114
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328513644
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328527469
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328526628
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328529606
PR Review Comment: https://git.openjdk.org/jdk/pull/15769#discussion_r1328531344
