<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