Quantcast

JDK 9 RFR of JDK-8169971: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java fails intermittently

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

JDK 9 RFR of JDK-8169971: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java fails intermittently

Amy Lu-2
Please review this test-only change.

tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java

This test fails intermittently on Windows platform in the final cleanup
step, @AfterClass, in which test tries to delete all dirs and files it
created. Though it’s okay to leave this cleanup work to jtreg, I still
make it done in test by making test to create everything under
./testoutput (testdir), and delete this testdir directly with
FileUtils.deleteFileTreeWithRetry(testdir).

Moreover, in FileUtils, as it tries to delete path with retry, we might
want to give it a chance to "retry" in case of DirectoryNotEmptyException.

bug: https://bugs.openjdk.java.net/browse/JDK-8169971
webrev: http://cr.openjdk.java.net/~amlu/8169971/webrev.00/

Thanks,
Amy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 9 RFR of JDK-8169971: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java fails intermittently

Paul Sandoz
Hi Amy,

FileUtils


-            } catch (NoSuchFileException | DirectoryNotEmptyException x) {
+            } catch (NoSuchFileException x) {


You removed the catch and rethrow of DirectoryNotEmptyException. The documentation of deleteFIleIfExistsWithRetry states:

  72     /**
  73      * Deletes a file, retrying if necessary.
  74      * No exception thrown if file doesn't exist.
  75      *
  76      * @param path  the file to delete
  77      *
  78      * @throws NoSuchFileException
  79      *         if the file does not exist (optional specific exception)
  80      * @throws DirectoryNotEmptyException
  81      *         if the file is a directory and could not otherwise be deleted
  82      *         because the directory is not empty (optional specific exception)
  83      * @throws IOException
  84      *         if an I/O error occurs
  85      */
  86     public static void deleteFileIfExistsWithRetry(Path path)


I dunno what it means by "(optional specific exception)”, but what happens now if deleteFileIfExistsWithRetry is called with a non-empty directory?

It’s amazing how hard it is to actually forcibly delete something. Such “simple” ( :-) ) utilities should be part of the JDK.



Do you think it’s simpler just to let jtreg cleanup? There are a few Mr.Jar tests that clean up after themselves but i wonder if it’s just simpler to let jtreg do it? any thoughts on that?

Paul.

> On 10 Apr 2017, at 19:15, Amy Lu <[hidden email]> wrote:
>
> Please review this test-only change.
>
> tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java
>
> This test fails intermittently on Windows platform in the final cleanup step, @AfterClass, in which test tries to delete all dirs and files it created. Though it’s okay to leave this cleanup work to jtreg, I still make it done in test by making test to create everything under ./testoutput (testdir), and delete this testdir directly with FileUtils.deleteFileTreeWithRetry(testdir).
>
> Moreover, in FileUtils, as it tries to delete path with retry, we might want to give it a chance to "retry" in case of DirectoryNotEmptyException.
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8169971
> webrev: http://cr.openjdk.java.net/~amlu/8169971/webrev.00/
>
> Thanks,
> Amy
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 9 RFR of JDK-8169971: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java fails intermittently

Alan Bateman
On 12/04/2017 17:36, Paul Sandoz wrote:

> Such “simple” ( :-) ) utilities should be part of the JDK.
>
I think this utility method in the test library may the pre-date in
jtreg that does retry. As to why we retry, then it's virus checkers and
the like locking files that are in use by the test. I don't know why
they are configured to run in test environments as they regularly cause
interference.

-Alan
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 9 RFR of JDK-8169971: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java fails intermittently

Chris Hegarty
In reply to this post by Paul Sandoz

> On 12 Apr 2017, at 17:36, Paul Sandoz <[hidden email]> wrote:
> ..
> Do you think it’s simpler just to let jtreg cleanup? There are a few Mr.Jar tests that clean up after themselves but i wonder if it’s just simpler to let jtreg do it? any thoughts on that?

I added these test library utility methods. Some times you actually
need to clean up, other times you can leave it to jtreg ( if the files
are all within the working/scratch directory ). Jtreg has similar
retry logic when deleting files on Windows.

It is common to see tests cleanup after themselves, as the original
author may have been running the test standalone, not in jtreg,
but we're mostly past that now.

In this particular case it may be ok to leave it to jtreg to do the
clean up.

-Chris.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 9 RFR of JDK-8169971: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java fails intermittently

Amy Lu-2
Thank you for the comments!

Yes, let's just make it simple, let jtreg to do the final clean up.

Please review the updated webrev:
http://cr.openjdk.java.net/~amlu/8169971/webrev.01/

(Tested on one win64 and one win32, loop run 500 times, all pass.)

Thanks,
Amy


On 4/13/17 3:16 AM, Chris Hegarty wrote:

>> On 12 Apr 2017, at 17:36, Paul Sandoz <[hidden email]> wrote:
>> ..
>> Do you think it’s simpler just to let jtreg cleanup? There are a few Mr.Jar tests that clean up after themselves but i wonder if it’s just simpler to let jtreg do it? any thoughts on that?
> I added these test library utility methods. Some times you actually
> need to clean up, other times you can leave it to jtreg ( if the files
> are all within the working/scratch directory ). Jtreg has similar
> retry logic when deleting files on Windows.
>
> It is common to see tests cleanup after themselves, as the original
> author may have been running the test standalone, not in jtreg,
> but we're mostly past that now.
>
> In this particular case it may be ok to leave it to jtreg to do the
> clean up.
>
> -Chris.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 9 RFR of JDK-8169971: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java fails intermittently

Chris Hegarty

> On 13 Apr 2017, at 11:28, Amy Lu <[hidden email]> wrote:
>
> Thank you for the comments!
>
> Yes, let's just make it simple, let jtreg to do the final clean up.
>
> Please review the updated webrev: http://cr.openjdk.java.net/~amlu/8169971/webrev.01/

Looks ok to me.

-Chris.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: JDK 9 RFR of JDK-8169971: tools/jlink/multireleasejar/JLinkMultiReleaseJarTest.java fails intermittently

Paul Sandoz

> On 13 Apr 2017, at 03:42, Chris Hegarty <[hidden email]> wrote:
>
>
>> On 13 Apr 2017, at 11:28, Amy Lu <[hidden email]> wrote:
>>
>> Thank you for the comments!
>>
>> Yes, let's just make it simple, let jtreg to do the final clean up.
>>
>> Please review the updated webrev: http://cr.openjdk.java.net/~amlu/8169971/webrev.01/
>
> Looks ok to me.
>

+1

Paul.
Loading...