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

Semyon Sadetsky semyon.sadetsky at oracle.com
Wed Jan 10 16:58:39 UTC 2018


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 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 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.addepalli at oracle.com 
> <mailto:krishna.addepalli at oracle.com>>; swing-dev at openjdk.java.net 
> <mailto: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; swing-dev at openjdk.java.net 
> <mailto: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/ 
> <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.addepalli at oracle.com 
> <mailto:krishna.addepalli at oracle.com>>; swing-dev at openjdk.java.net 
> <mailto: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; swing-dev at openjdk.java.net 
> <mailto: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/ 
> <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.addepalli at oracle.com 
> <mailto:krishna.addepalli at oracle.com>>; swing-dev at openjdk.java.net 
> <mailto: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 <krishna.addepalli at oracle.com>
>     <mailto:krishna.addepalli at oracle.com>; swing-dev at openjdk.java.net
>     <mailto: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:
>         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
>

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


More information about the swing-dev mailing list