Re: [10][11][JDK-8194044] Regression manual Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails

Thu, 11 Jan 2018 19:08:04 -0800 

+1

--Semyon


On 01/11/2018 06:44 PM, Krishna Addepalli wrote:


Hi Semyon,
Per our conversation, I have removed the loop, and also added the test for FileSystemView.getSystemDisplayName function as well.
Here is the new webrev: http://cr.openjdk.java.net/~kaddepalli/8194044/webrev04/ <http://cr.openjdk.java.net/%7Ekaddepalli/8194044/webrev04/> 

Thanks,

Krishna

*From:*Krishna Addepalli
*Sent:* Wednesday, January 10, 2018 11:41 PM
*To:* Semyon Sadetsky <[email protected]>; Jayathirth D V <[email protected]>; [email protected] *Subject:* RE: <Swing Dev> [10][11][JDK-8194044] Regression manual Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails 

Hi Semyon,
The idea was to test the ShellFolder path in the function “Win32ShellFolderManager2.isFileSystemRoot” function, which was being reported as false, before my fix.
Secondly, “getShellFolder” function is package specific function, so I cannot invoke it directly from outside, so had to get a path to the default folder, and then keep getting the parent directory from there, till I come to root drive i.e “C:\\”.
I adapted the code from the test that Prasanta suggested(have test/jdk/javax/swing/JFileChooser/6945316/bug6945316.java) which tests isFileSystemRoot, and simplified it, so left in the loop. 

Thanks,

Krishna

*From:*Semyon Sadetsky
*Sent:* Wednesday, January 10, 2018 10:29 PM
*To:* Jayathirth D V <[email protected] <mailto:[email protected]>>; Krishna Addepalli <[email protected] <mailto:[email protected]>>; [email protected] <mailto:[email protected]> *Subject:* Re: <Swing Dev> [10][11][JDK-8194044] Regression manual Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails 

Hi Krishna,
The fix looks good. But I did not understand the test. In my understanding it should check that the next is not en empty:
FileSystemView.getFileSystemView().getSystemDisplayName(FileSystemView.getFileSystemView().getShellFolder(new File("C:\\"))) 

And why do you run the test 50 times?

--Semyon

On 1/10/2018 7:57 AM, Jayathirth D V wrote:

    Hi Krishna,

    Changes are fine.

    Thanks,

    Jay

    *From:*Krishna Addepalli
    *Sent:* Wednesday, January 10, 2018 8:17 PM
    *To:* Jayathirth D V; [email protected]
    <mailto:[email protected]>
    *Subject:* RE: <Swing Dev> [10][11][JDK-8194044] Regression manual
    Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails

    Hi Jay,

    Thanks for the suggestions, and I have created a new webrev here:
    http://cr.openjdk.java.net/~kaddepalli/8194044/webrev03/
    <http://cr.openjdk.java.net/%7Ekaddepalli/8194044/webrev03/>

    However, I don’t think Test if is a typo. It can be read as is and
    still makes sense.

    Regards,

    Krishna

    *From:*Jayathirth D V
    *Sent:* Wednesday, January 10, 2018 6:18 PM
    *To:* Krishna Addepalli <[email protected]
    <mailto:[email protected]>>; [email protected]
    <mailto:[email protected]>
    *Subject:* RE: <Swing Dev> [10][11][JDK-8194044] Regression manual
    Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails

    Hi Krishna,

    Please find my inputs:

    1)There is no need for Starting and Ending year in copyright of
    test case as it is a new file. Adding just 2018 would be enough.

    2)Typo in jtreg summary : “Test if” it should be “Tests if”.

    3)For multiline comments using /*..*/,  First line should be left
    empty in a block comment at Line no 24 & 42 and last line of block
    comment should have proper indentation at Line no 30. Java coding
    convention for comments :
    http://www.oracle.com/technetwork/java/codeconventions-141999.html

    Thanks,

    Jay

    *From:*Krishna Addepalli
    *Sent:* Wednesday, January 10, 2018 6:00 PM
    *To:* Jayathirth D V; [email protected]
    <mailto:[email protected]>
    *Subject:* RE: <Swing Dev> [10][11][JDK-8194044] Regression manual
    Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails

    Hi Jay,

    Thanks for your time and review. I have incorporated your review
    comments and created a new webrev here:
    http://cr.openjdk.java.net/~kaddepalli/8194044/webrev02/
    <http://cr.openjdk.java.net/%7Ekaddepalli/8194044/webrev02/>

    Thanks,

    Krishna

    *From:*Jayathirth D V
    *Sent:* Wednesday, January 10, 2018 4:16 PM
    *To:* Krishna Addepalli <[email protected]
    <mailto:[email protected]>>; [email protected]
    <mailto:[email protected]>
    *Subject:* RE: <Swing Dev> [10][11][JDK-8194044] Regression manual
    Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails

    Hi Krishna,

    Please find my inputs:

    Test case needs to be updated with minor changes:

    1)Copyright information needs to be added at the start of test file.

    2)Jtreg test summary needs to be updated to mention what the test
    is trying to achieve instead of JBS bug title.

    3)Also since this test is specific to Windows we can remove the OS
    test code present in test case with Jtreg @requires flag:

    Instead of using the below code we can use @requires (os.family ==
    "windows")

                if (OSInfo.getOSType() != OSInfo.OSType.WINDOWS) {

    System.out.println("The test is suitable only for Windows OS.
    Skipped.");

    return;

    }

    4)For multiline comments at Line no 1 & 26 in test case please
    update the comment syntax to use :

    /*

     *

     *

     */

    Thanks,

    Jay

    *From:*Krishna Addepalli
    *Sent:* Tuesday, January 09, 2018 2:30 PM
    *To:* Prasanta Sadhukhan; [email protected]
    <mailto:[email protected]>
    *Subject:* Re: <Swing Dev> [10][11][JDK-8194044] Regression manual
    Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails

    Hi Prasanta,Sergey,

    I have added a testcase along with the changes and created a new
    webrev : http://cr.openjdk.java.net/~kaddepalli/8194044/webrev01/
    <http://cr.openjdk.java.net/%7Ekaddepalli/8194044/webrev01/>

    Please review this and provide your comments.

    Thanks,

    Krishna

    *From:*Prasanta Sadhukhan
    *Sent:* Thursday, January 4, 2018 10:54 AM
    *To:* Krishna Addepalli <[email protected]
    <mailto:[email protected]>>; [email protected]
    <mailto:[email protected]>
    *Subject:* Re: <Swing Dev> [10][11][JDK-8194044] Regression manual
    Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails

    A regression test is added to prove that before your fix, java was
    failing and after your fix is applied, it is passing so it is not
    related to older/newer java versions. I saw we already have
    test/jdk/javax/swing/JFileChooser/6945316/bug6945316.java which
    tests isFileSystemRoot. Maybe, we can do something similar if it's
    not too much of a task.

    Regards
    Prasanta

    On 1/3/2018 5:13 PM, Krishna Addepalli wrote:

        Thanks for the review Prasanta. However, I don’t see a point
        to write a test case for isFileSystemRoot(), since, it is not
        going to fail on any (older/newer) java versions, and it was
        only introduced because of the fix for JDK-8175015.

        Let me know if you think otherwise.

        Regards,

        Krishna

        *From:*Prasanta Sadhukhan
        *Sent:* Wednesday, January 3, 2018 11:27 AM
        *To:* Krishna Addepalli <[email protected]>
        <mailto:[email protected]>;
        [email protected] <mailto:[email protected]>
        *Subject:* Re: <Swing Dev> [10][11][JDK-8194044] Regression
        manual Test
        javax/swing/JFileChooser/8067660/FileChooserTest.java fails

        Fix looks fine. But I guess, it is possible to add a automated
        regression test to it utilising isFileSystemRoot().

        Regards
        Prasanta

        On 1/2/2018 4:31 PM, Krishna Addepalli wrote:

            Hi All,

            Please review a fix for bug:

            Bug: JDK-8194044 :
            https://bugs.openjdk.java.net/browse/JDK-8194044

            Webrev:
            http://cr.openjdk.java.net/~kaddepalli/8194044/webrev00/
            <http://cr.openjdk.java.net/%7Ekaddepalli/8194044/webrev00/>

            This was caused due to the fix for JDK-8175015, in which
            the line 446  in Win32ShellFolderManager2.java was changed
            from “getDrives()” to Win32ShellFolder2.listRoots().

            While the earlier function returns an object of
            Win32ShellFolder2, the latter returns an array of Files.

            The condition on line 450: “return
            (sf.isFileSystem()&&sf.parent != null &&
            sf.parent.equals(Win32ShellFolder2.listRoots())” was
            returning false because of the wrong object being passed.
            Earlier it was a Win32ShellFolder2 object, and the
            comparision was done properly, but with the changes, the
            equals fucnction was receiving a file array object, and
            hence it was immediately returning false, leading to the
            problem of empty strings being shown for Root drives.

            The fix is to replace “Win32ShellFolder2.listRoots()” with
            “getDrives()” function. With this fix, the regression is
            addressed, as well as the original JDK-8175015 which was a
            memory leak issue.

            Thanks,

            Krishna

Reply via email to