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

Semyon Sadetsky semyon.sadetsky at oracle.com
Fri Jan 12 03:07:04 UTC 2018


+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 <semyon.sadetsky at oracle.com>; Jayathirth D V 
> <jayathirth.d.v 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 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 <jayathirth.d.v at oracle.com 
> <mailto:jayathirth.d.v at oracle.com>>; 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,
>
> 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
>     <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 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/20180111/c822b477/attachment.html>


More information about the swing-dev mailing list