<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