<Swing Dev> [10][11][JDK-8194044] Regression manual Test javax/swing/JFileChooser/8067660/FileChooserTest.java fails

[Krishna Addepalli]

Hi Jay,

 

Thanks for the suggestions, and I have created a new webrev here: http://cr.openjdk.java.net/~kaddepalli/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.addepalli at oracle.com>; swing-dev at 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; HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at 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/

 

Thanks,

Krishna

 

From: Jayathirth D V 
Sent: Wednesday, January 10, 2018 4:16 PM
To: Krishna Addepalli <HYPERLINK "mailto:krishna.addepalli at oracle.com"krishna.addepalli at oracle.com>; HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at 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; HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at 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/

 

Please review this and provide your comments.

 

Thanks,

Krishna

 

From: Prasanta Sadhukhan 
Sent: Thursday, January 4, 2018 10:54 AM
To: Krishna Addepalli <HYPERLINK "mailto:krishna.addepalli at oracle.com"krishna.addepalli at oracle.com>; HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at 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 HYPERLINK "mailto:krishna.addepalli at oracle.com"<krishna.addepalli at oracle.com>; HYPERLINK "mailto:swing-dev at openjdk.java.net"swing-dev at 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: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8194044/webrev00/"http://cr.openjdk.java.net/~kaddepalli/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

 

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20180110/c0c3a0ee/attachment.html>