[PATCH] Review Request for 8009258: TEST_BUG: java/io/pathNames/GeneralWin32.java fails intermittently
Hi, Please help to review fix below for bug 8009258 <https://jbs.oracle.com/bugs/browse/JDK-8009258>, TEST_BUG:java/io/pathNames/GeneralWin32.java fails intermittently. http://cr.openjdk.java.net/~ewang/8009258/webrev.00/ The File.canRead() method should not be used to check read permission of a directory. Thanks, Eric
On 04/03/2013 17:32, Eric Wang wrote:
Hi,
Please help to review fix below for bug 8009258 <https://jbs.oracle.com/bugs/browse/JDK-8009258>, TEST_BUG:java/io/pathNames/GeneralWin32.java fails intermittently. http://cr.openjdk.java.net/~ewang/8009258/webrev.00/
The File.canRead() method should not be used to check read permission of a directory.
Thanks, Eric I wonder if it would be better to change this test so that it doesn't even attempt to poke around in these directories. I suggest this because there may be other activity going on at the same time. See also 8004096 where the test is running in agentvm mode and is straying into the directory used by another agent VM.
-Alan
Hi, Please review the code change, I have updated the test to make sure test only access files and directories created by itself. http://cr.openjdk.java.net/~ewang/8009258/webrev.01/ Here is the execution result: http://cr.openjdk.java.net/~ewang/8009258/GeneralWin32.jtr Thanks, Eric On 2013/3/5 1:39, Alan Bateman wrote:
On 04/03/2013 17:32, Eric Wang wrote:
Hi,
Please help to review fix below for bug 8009258 <https://jbs.oracle.com/bugs/browse/JDK-8009258>, TEST_BUG:java/io/pathNames/GeneralWin32.java fails intermittently. http://cr.openjdk.java.net/~ewang/8009258/webrev.00/
The File.canRead() method should not be used to check read permission of a directory.
Thanks, Eric I wonder if it would be better to change this test so that it doesn't even attempt to poke around in these directories. I suggest this because there may be other activity going on at the same time. See also 8004096 where the test is running in agentvm mode and is straying into the directory used by another agent VM.
-Alan
Including more reviewer... Thanks, Eric On 2013/3/13 14:28, Eric Wang wrote:
Hi,
Please review the code change, I have updated the test to make sure test only access files and directories created by itself. http://cr.openjdk.java.net/~ewang/8009258/webrev.01/
Here is the execution result: http://cr.openjdk.java.net/~ewang/8009258/GeneralWin32.jtr
Thanks, Eric On 2013/3/5 1:39, Alan Bateman wrote:
On 04/03/2013 17:32, Eric Wang wrote:
Hi,
Please help to review fix below for bug 8009258 <https://jbs.oracle.com/bugs/browse/JDK-8009258>, TEST_BUG:java/io/pathNames/GeneralWin32.java fails intermittently. http://cr.openjdk.java.net/~ewang/8009258/webrev.00/
The File.canRead() method should not be used to check read permission of a directory.
Thanks, Eric I wonder if it would be better to change this test so that it doesn't even attempt to poke around in these directories. I suggest this because there may be other activity going on at the same time. See also 8004096 where the test is running in agentvm mode and is straying into the directory used by another agent VM.
-Alan
Hi Eric, Thanks for fixing the test failures. I recently reviewed your changes. And I like your idea to add a base dir to restrict the test only touching files/directories that are created by itself to avoid the interferences from the OS or other test activities. And in Line 341 of General.java, I notice you make the code return if it tries to test baseDir or its ascendant directories, which reduces the test coverage. Since GeneralWin32.java knows its max tree depth, I think you can make the baseDir deep enough in the prepared directory structure so that the test can still run inside the testing directories even if it visits the baseDir's ascendant directories. One idea is to make the max depth as a parameter of initTestData(), and this method can intelligently return an appropriate baseDir basing on it. After the above change, you can move the baseDir and userDir back to GeneralWin32.java to make the two classes loosely coupled. In the changes, the usages of NIO classes are not necessary. The test is against java.io packages, and it is better to keep it clean in case it might be used to test old jdk version which does not have NIO. Before the test ends, it is better to clean the testing files and directories which are created at the beginning. Thanks! -Dan On 03/12/2013 11:28 PM, Eric Wang wrote:
Hi,
Please review the code change, I have updated the test to make sure test only access files and directories created by itself. http://cr.openjdk.java.net/~ewang/8009258/webrev.01/
Here is the execution result: http://cr.openjdk.java.net/~ewang/8009258/GeneralWin32.jtr
Thanks, Eric On 2013/3/5 1:39, Alan Bateman wrote:
On 04/03/2013 17:32, Eric Wang wrote:
Hi,
Please help to review fix below for bug 8009258 <https://jbs.oracle.com/bugs/browse/JDK-8009258>, TEST_BUG:java/io/pathNames/GeneralWin32.java fails intermittently. http://cr.openjdk.java.net/~ewang/8009258/webrev.00/
The File.canRead() method should not be used to check read permission of a directory.
Thanks, Eric I wonder if it would be better to change this test so that it doesn't even attempt to poke around in these directories. I suggest this because there may be other activity going on at the same time. See also 8004096 where the test is running in agentvm mode and is straying into the directory used by another agent VM.
-Alan
On 17/04/2013 07:36, Dan Xu wrote:
:
In the changes, the usages of NIO classes are not necessary. The test is against java.io packages, and it is better to keep it clean in case it might be used to test old jdk version which does not have NIO.
I briefly looked at it and I think the usage okay is because it just makes it easier to check the results and also to keep the directory walk from wandering into other parts of the file system. If it changed what was being tested then I would agree that it shouldn't be used. -Alan
On 04/17/2013 02:15 AM, Alan Bateman wrote:
On 17/04/2013 07:36, Dan Xu wrote:
:
In the changes, the usages of NIO classes are not necessary. The test is against java.io packages, and it is better to keep it clean in case it might be used to test old jdk version which does not have NIO.
I briefly looked at it and I think the usage okay is because it just makes it easier to check the results and also to keep the directory walk from wandering into other parts of the file system. If it changed what was being tested then I would agree that it shouldn't be used.
-Alan
I see. My thought is that the relative path between baseDir and userDir can be calculated inside initTestData() when it constructs the baseDir. But I agree it is not an issue here. Thanks! -Dan
Hi Dan & All, I have updated the test based on your comments, Can you please review the fix? Thanks! http://cr.openjdk.java.net/~ewang/8009258/webrev.02/ Regards, Eric On 2013/4/17 14:36, Dan Xu wrote:
Hi Eric,
Thanks for fixing the test failures. I recently reviewed your changes. And I like your idea to add a base dir to restrict the test only touching files/directories that are created by itself to avoid the interferences from the OS or other test activities.
And in Line 341 of General.java, I notice you make the code return if it tries to test baseDir or its ascendant directories, which reduces the test coverage. Since GeneralWin32.java knows its max tree depth, I think you can make the baseDir deep enough in the prepared directory structure so that the test can still run inside the testing directories even if it visits the baseDir's ascendant directories. One idea is to make the max depth as a parameter of initTestData(), and this method can intelligently return an appropriate baseDir basing on it.
After the above change, you can move the baseDir and userDir back to GeneralWin32.java to make the two classes loosely coupled.
In the changes, the usages of NIO classes are not necessary. The test is against java.io packages, and it is better to keep it clean in case it might be used to test old jdk version which does not have NIO.
Before the test ends, it is better to clean the testing files and directories which are created at the beginning. Thanks!
-Dan
On 03/12/2013 11:28 PM, Eric Wang wrote:
Hi,
Please review the code change, I have updated the test to make sure test only access files and directories created by itself. http://cr.openjdk.java.net/~ewang/8009258/webrev.01/
Here is the execution result: http://cr.openjdk.java.net/~ewang/8009258/GeneralWin32.jtr
Thanks, Eric On 2013/3/5 1:39, Alan Bateman wrote:
On 04/03/2013 17:32, Eric Wang wrote:
Hi,
Please help to review fix below for bug 8009258 <https://jbs.oracle.com/bugs/browse/JDK-8009258>, TEST_BUG:java/io/pathNames/GeneralWin32.java fails intermittently. http://cr.openjdk.java.net/~ewang/8009258/webrev.00/
The File.canRead() method should not be used to check read permission of a directory.
Thanks, Eric I wonder if it would be better to change this test so that it doesn't even attempt to poke around in these directories. I suggest this because there may be other activity going on at the same time. See also 8004096 where the test is running in agentvm mode and is straying into the directory used by another agent VM.
-Alan
On 24/05/2013 05:19, Eric Wang wrote:
Hi Dan & All,
I have updated the test based on your comments, Can you please review the fix? Thanks! http://cr.openjdk.java.net/~ewang/8009258/webrev.02/ I think this is okay. Dan is going to sponsor this one.
-Alan
On 05/24/2013 07:07 AM, Alan Bateman wrote:
On 24/05/2013 05:19, Eric Wang wrote:
Hi Dan & All,
I have updated the test based on your comments, Can you please review the fix? Thanks! http://cr.openjdk.java.net/~ewang/8009258/webrev.02/ I think this is okay. Dan is going to sponsor this one.
-Alan
Hi Eric, The changes look good. I am going to push it for you today. Thanks for your effort! -Dan
participants (3)
-
Alan Bateman
-
Dan Xu
-
Eric Wang