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

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Krishna Addepalli

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/

 

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 | Threaded
Open this post in threaded view
|

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

prasanta sadhukhan

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/

 

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 | Threaded
Open this post in threaded view
|

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

Krishna Addepalli

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 <[hidden email]>; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

prasanta sadhukhan

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 [hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Krishna Addepalli

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 <[hidden email]>; [hidden email]
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 [hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Jayathirth D v

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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Krishna Addepalli

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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Jayathirth D v

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; [hidden email]
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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

Re: <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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Jayathirth D v

Hi Krishna,

 

Changes are fine.

 

Thanks,

Jay

 

From: Krishna Addepalli
Sent: Wednesday, January 10, 2018 8:17 PM
To: Jayathirth D V; [hidden email]
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/

 

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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Semyon Sadetsky
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; [hidden email]
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/

 

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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Krishna Addepalli

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 <[hidden email]>; Krishna Addepalli <[hidden email]>; [hidden email]
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; [hidden email]
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/

 

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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Krishna Addepalli

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/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, January 10, 2018 11:41 PM
To: Semyon Sadetsky <[hidden email]>; Jayathirth D V <[hidden email]>; [hidden email]
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 <[hidden email]>; Krishna Addepalli <[hidden email]>; [hidden email]
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; [hidden email]
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/

 

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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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 | Threaded
Open this post in threaded view
|

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

Semyon Sadetsky

+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/

 

Thanks,

Krishna

 

From: Krishna Addepalli
Sent: Wednesday, January 10, 2018 11:41 PM
To: Semyon Sadetsky [hidden email]; Jayathirth D V [hidden email]; [hidden email]
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 <[hidden email]>; Krishna Addepalli <[hidden email]>; [hidden email]
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; [hidden email]
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/

 

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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <[hidden email]>; [hidden email]
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; [hidden email]
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 <
[hidden email]>; [hidden email]
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
[hidden email]; [hidden email]
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/

 

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