Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

[Paul Sandoz]

> On 18 Jun 2017, at 19:47, Hamlin Li  wrote:
> 
> 
> 
> On 2017/6/17 1:31, Paul Sandoz wrote:
>>> On 14 Jun 2017, at 23:29, Hamlin Li  wrote:
>>> 
>>> Hi Alan, Paul,
>>> 
>>> Thank you for review, new webrev at: 
>>> http://cr.openjdk.java.net/~mli/8181478/webrev.01/
>>> 
>>> Please also check my comments inline.
>>> 
>>> 
>>> On 2017/6/15 1:28, Alan Bateman wrote:
 On 14/06/2017 18:20, Paul Sandoz wrote:
>> On 12 Jun 2017, at 01:00, Hamlin Li  wrote:
>> 
>> Would you please review the below patch?
>> 
>> bug: https://bugs.openjdk.java.net/browse/JDK-8181478
>> 
>> webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/
>> 
> It took me a few moments to grok the NonExistentDriver behaviour. If i 
> got it correct, this is checking that deleteOnExit is working correctly?
> 
> If so it might be clearer to make this is a separate test that is run as 
> a driver and a test with different arguments performing the action and 
> then the checking.
>>> Agree, it's not that clear. And I just found out there is another test 
>>> java/io/File/DeleteOnExit.java testing the File.deleteOnExit. so I just 
>>> remove NonExistentDriver.java and related code.
>> Do you still require the following:
>> 
>>   48 static File nonDir = new File("x.Basic.nonDir");
>> ...
>>   93 nonDir.delete();
>> ...
>>  133 if (!nonDir.mkdir()) {
>>  134 fail(nonDir, "could not create");
>>  135 }
>>  136 if (!nonDir.exists() || !nonDir.isDirectory()) {
>>  137 fail(nonDir, "not created");
>>  138 }
>> 
>> since it is now just duplicating the assertions from the results of a call 
>> to show.
> Hi Paul,
> 
> Thank you for detailed review.
> I'm not sure if I understand you correctly, I think it still needs to verify 
> the creation of a directory, so I merged the test of dir and nonDir in new 
> webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.02/
> 

That’s better +1

Paul.

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

[Hamlin Li]

On 2017/6/17 1:31, Paul Sandoz wrote:

On 14 Jun 2017, at 23:29, Hamlin Li  wrote:

Hi Alan, Paul,

Thank you for review, new webrev at: 
http://cr.openjdk.java.net/~mli/8181478/webrev.01/

Please also check my comments inline.


On 2017/6/15 1:28, Alan Bateman wrote:

On 14/06/2017 18:20, Paul Sandoz wrote:

On 12 Jun 2017, at 01:00, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/


It took me a few moments to grok the NonExistentDriver behaviour. If i got it 
correct, this is checking that deleteOnExit is working correctly?

If so it might be clearer to make this is a separate test that is run as a 
driver and a test with different arguments performing the action and then the 
checking.

Agree, it's not that clear. And I just found out there is another test 
java/io/File/DeleteOnExit.java testing the File.deleteOnExit. so I just remove 
NonExistentDriver.java and related code.

Do you still require the following:

   48 static File nonDir = new File("x.Basic.nonDir");
...
   93 nonDir.delete();
...
  133 if (!nonDir.mkdir()) {
  134 fail(nonDir, "could not create");
  135 }
  136 if (!nonDir.exists() || !nonDir.isDirectory()) {
  137 fail(nonDir, "not created");
  138 }

since it is now just duplicating the assertions from the results of a call to 
show.

Hi Paul,

Thank you for detailed review.
I'm not sure if I understand you correctly, I think it still needs to 
verify the creation of a directory, so I merged the test of dir and 
nonDir in new webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.02/


Thank you
-Hamlin


Paul.


I agree. Also in FileOpenTest then it can use APIs to set the hidden attribute, 
no need to launch attrib.exe each time.

This is a test running only on windows.
I'm not sure if I understand you correctly. In new patch I use Files.setAttribute(path, 
"dos:hidden", true/false); to hide/un-hide a file, and use File.setReadOnly() 
to make a file read only.

Thank you
-Hamlin

-Alan

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

[Paul Sandoz]

> On 14 Jun 2017, at 23:29, Hamlin Li  wrote:
> 
> Hi Alan, Paul,
> 
> Thank you for review, new webrev at: 
> http://cr.openjdk.java.net/~mli/8181478/webrev.01/
> 
> Please also check my comments inline.
> 
> 
> On 2017/6/15 1:28, Alan Bateman wrote:
>> 
>> On 14/06/2017 18:20, Paul Sandoz wrote:
 On 12 Jun 2017, at 01:00, Hamlin Li  wrote:
 
 Would you please review the below patch?
 
 bug: https://bugs.openjdk.java.net/browse/JDK-8181478
 
 webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/
 
>>> It took me a few moments to grok the NonExistentDriver behaviour. If i got 
>>> it correct, this is checking that deleteOnExit is working correctly?
>>> 
>>> If so it might be clearer to make this is a separate test that is run as a 
>>> driver and a test with different arguments performing the action and then 
>>> the checking.
> Agree, it's not that clear. And I just found out there is another test 
> java/io/File/DeleteOnExit.java testing the File.deleteOnExit. so I just 
> remove NonExistentDriver.java and related code.

Do you still require the following:

  48 static File nonDir = new File("x.Basic.nonDir");
...
  93 nonDir.delete();
...
 133 if (!nonDir.mkdir()) {
 134 fail(nonDir, "could not create");
 135 }
 136 if (!nonDir.exists() || !nonDir.isDirectory()) {
 137 fail(nonDir, "not created");
 138 }

since it is now just duplicating the assertions from the results of a call to 
show.

Paul.

>> I agree. Also in FileOpenTest then it can use APIs to set the hidden 
>> attribute, no need to launch attrib.exe each time.
> This is a test running only on windows.
> I'm not sure if I understand you correctly. In new patch I use 
> Files.setAttribute(path, "dos:hidden", true/false); to hide/un-hide a file, 
> and use File.setReadOnly() to make a file read only.
> 
> Thank you
> -Hamlin
>> 
>> -Alan
> 

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

[Hamlin Li]

Ping.

Thank you

-Hamlin


On 2017/6/15 14:29, Hamlin Li wrote:

Hi Alan, Paul,

Thank you for review, new webrev at: 
http://cr.openjdk.java.net/~mli/8181478/webrev.01/


Please also check my comments inline.


On 2017/6/15 1:28, Alan Bateman wrote:


On 14/06/2017 18:20, Paul Sandoz wrote:

On 12 Jun 2017, at 01:00, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/

It took me a few moments to grok the NonExistentDriver behaviour. If 
i got it correct, this is checking that deleteOnExit is working 
correctly?


If so it might be clearer to make this is a separate test that is 
run as a driver and a test with different arguments performing the 
action and then the checking.
Agree, it's not that clear. And I just found out there is another test 
java/io/File/DeleteOnExit.java testing the File.deleteOnExit. so I 
just remove NonExistentDriver.java and related code.
I agree. Also in FileOpenTest then it can use APIs to set the hidden 
attribute, no need to launch attrib.exe each time.

This is a test running only on windows.
I'm not sure if I understand you correctly. In new patch I use 
Files.setAttribute(path, "dos:hidden", true/false); to hide/un-hide a 
file, and use File.setReadOnly() to make a file read only.


Thank you
-Hamlin


-Alan

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

[Hamlin Li]

Hi Alan, Paul,

Thank you for review, new webrev at: 
http://cr.openjdk.java.net/~mli/8181478/webrev.01/


Please also check my comments inline.


On 2017/6/15 1:28, Alan Bateman wrote:


On 14/06/2017 18:20, Paul Sandoz wrote:

On 12 Jun 2017, at 01:00, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/

It took me a few moments to grok the NonExistentDriver behaviour. If 
i got it correct, this is checking that deleteOnExit is working 
correctly?


If so it might be clearer to make this is a separate test that is run 
as a driver and a test with different arguments performing the action 
and then the checking.
Agree, it's not that clear. And I just found out there is another test 
java/io/File/DeleteOnExit.java testing the File.deleteOnExit. so I just 
remove NonExistentDriver.java and related code.
I agree. Also in FileOpenTest then it can use APIs to set the hidden 
attribute, no need to launch attrib.exe each time.

This is a test running only on windows.
I'm not sure if I understand you correctly. In new patch I use 
Files.setAttribute(path, "dos:hidden", true/false); to hide/un-hide a 
file, and use File.setReadOnly() to make a file read only.


Thank you
-Hamlin


-Alan

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

[Alan Bateman]

On 14/06/2017 18:20, Paul Sandoz wrote:

On 12 Jun 2017, at 01:00, Hamlin Li  wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/


It took me a few moments to grok the NonExistentDriver behaviour. If i got it 
correct, this is checking that deleteOnExit is working correctly?

If so it might be clearer to make this is a separate test that is run as a 
driver and a test with different arguments performing the action and then the 
checking.

I agree. Also in FileOpenTest then it can use APIs to set the hidden 
attribute, no need to launch attrib.exe each time.


-Alan

Re: (10) RFR of JDK-8181478, Refactor java/io shell tests to plain java tests

[Paul Sandoz]

> On 12 Jun 2017, at 01:00, Hamlin Li  wrote:
> 
> Would you please review the below patch?
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8181478
> 
> webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/
> 

It took me a few moments to grok the NonExistentDriver behaviour. If i got it 
correct, this is checking that deleteOnExit is working correctly?

If so it might be clearer to make this is a separate test that is run as a 
driver and a test with different arguments performing the action and then the 
checking.

Paul.

(10) RFR of JDK-8181478,Refactor java/io shell tests to plain java tests

[Hamlin Li]

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8181478

webrev: http://cr.openjdk.java.net/~mli/8181478/webrev.00/


Thank you

-Hamlin