RFR: 8326606: Test javax/swing/text/BoxView/6494356/bug6494356.java performs a synchronization on a value based class

[Alexey Ivanov]

On Thu, 7 Mar 2024 03:16:20 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> wrote:

> OPensource BoxView test

Changes requested by aivanov (Reviewer).

Changes requested by aivanov (Reviewer).

Changes requested by aivanov (Reviewer).

test/jdk/javax/swing/text/BoxView/bug6494356.html line 1:

> 1: <p>Paragraph</p>

Since the test requires a supporting file, does it make sense to put the test in a folder `6494356/bug6494356.java`?

I wanted to asked whether an external file is needed? I answered this question: yes, it's needed. The test must use `setPage` which loads the file or URL asynchronously.

At the same time, the file to load can be created on the fly in the scratch directory which is the current directory when the test is run by jtreg.

Thus, an additional supporting file can be removed. Could update the test to create the file as a temporary file, please?

test/jdk/javax/swing/text/BoxView/bug6494356.java line 34:

> 32: import java.beans.PropertyChangeEvent;
> 33: import java.beans.PropertyChangeListener;
> 34: import java.io.IOException;

`IOException` is unused.

test/jdk/javax/swing/text/BoxView/bug6494356.java line 56:

> 54:             public void run() {
> 55:                 ep = new JEditorPane();
> 56:                 ep.setEditorKitForContentType("text/html", editorKit);

Suggestion:

                ep.setEditorKitForContentType("text/html", new MyEditorKit());

The instance of the custom `EditorKit` can be created here. I think it makes the test clearer.

test/jdk/javax/swing/text/BoxView/bug6494356.java line 72:

> 70:                 f.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
> 71:                 f.setVisible(true);
> 72:                 File file = new File("file:" + testSrc + "/" + "bug6494356.html");

Where does this file come from?

As the first step, you should write the file into *the current directory* with the content of `bug6494356.html`, that is `<p>Paragraph</p>`.


        final Path file = Path.of("bug6494356.html");
        try (Writer writer = Files.newBufferedWriter(file)) {
            writer.write("<p>Paragraph</p>");
        }


Then you load the file:


                    ep.setPage("file:" + file);


In the end of the test, you may remove the created file:


        Files.delete(file);

test/jdk/javax/swing/text/BoxView/bug6494356.java line 79:

> 77:                     try (Writer writer = Files.newBufferedWriter(file)) {
> 78:                         writer.write("<p>Paragraph</p>");
> 79:                     } catch (Exception e) {

I suggest creating the file *before* calling `SwingUtilities.invokeLater`.

If it throws an exception, the test fails right away, you won't even run the code inside `invokeLater`.

test/jdk/javax/swing/text/BoxView/bug6494356.java line 117:

> 115:         }
> 116: 
> 117:         static ViewFactory viewFactory = new MyViewFactory();

This should be a `final` field of `MyEditorKit`, no need to make it `static`.

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

PR Review: https://git.openjdk.org/jdk/pull/18147#pullrequestreview-1922519738
PR Review: https://git.openjdk.org/jdk/pull/18147#pullrequestreview-1922960768
PR Review: https://git.openjdk.org/jdk/pull/18147#pullrequestreview-1923181380
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516212830
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516209847
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516204845
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516478899
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516606920
PR Review Comment: https://git.openjdk.org/jdk/pull/18147#discussion_r1516208607