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

Wed, 10 Jan 2018 08:59:49 -0800 

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; swing-dev@openjdk.java.net

*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 <krishna.addepa...@oracle.com 
<mailto:krishna.addepa...@oracle.com>>; swing-dev@openjdk.java.net 
<mailto:swing-dev@openjdk.java.net>
*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; swing-dev@openjdk.java.net 
<mailto:swing-dev@openjdk.java.net>
*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 <krishna.addepa...@oracle.com 
<mailto:krishna.addepa...@oracle.com>>; swing-dev@openjdk.java.net 
<mailto:swing-dev@openjdk.java.net>
*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; swing-dev@openjdk.java.net 
<mailto:swing-dev@openjdk.java.net>
*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 <krishna.addepa...@oracle.com 
<mailto:krishna.addepa...@oracle.com>>; swing-dev@openjdk.java.net 
<mailto:swing-dev@openjdk.java.net>
*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 <krishna.addepa...@oracle.com>
    <mailto:krishna.addepa...@oracle.com>; swing-dev@openjdk.java.net
    <mailto:swing-dev@openjdk.java.net>
    *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