Add TestFileUtils.deleteDirWithRetry method.#2946
Add TestFileUtils.deleteDirWithRetry method.#2946labkey-danield wants to merge 13 commits intorelease26.3-SNAPSHOTfrom
Conversation
No need to wait for the search indexer.
…#7578) #### Rationale We read transform scripts during assay design save. We open but don't close the file. This is impolite, and Windows automated tests have been noticing. https://teamcity.labkey.org/buildConfiguration/LabKey_263Release_Premium_CommunitySqlserver_DailyCSqlserver/3929893?buildTab=tests&status=failed&expandedTest=build%3A%28id%3A3929893%29%2Cid%3A2000000048 #### Related Pull Requests - LabKey/testAutomation#2946 #### Changes - try-with-resources is the polite way to handle InputStreams <!-- list of standard tasks (remove this comment to enable) #### Tasks 📍 - [ ] Manual Testing - [ ] Needs Automation - [ ] Verify Fix -->
…egression to what it was originally. Move the delete and retry code in to TestFileUtils.deleteDirWithRetry
labkey-danield
changed the title
Dump the heap if running on TeamCity. Get the list of running processes if running on Linux as well.
| pb.redirectErrorStream(true); | ||
|
|
||
| // Tried to use "try (Process p = pb.start()) {" without the finally block but our build | ||
| // system didn't like that and complained that Process doesn't implement closeable (it does). |
There was a problem hiding this comment.
Process started implementing Closeable in Java 26. We're using Java 25.
|
|
||
| FileUtils.deleteDirectory(dir); | ||
| LOG.info(String.format("Deletion of directory %s was successful.", dir)); | ||
| break; |
There was a problem hiding this comment.
Consider making this return. If you do that, you can eliminate the if (attempt == 10) statement below and move its body outside of the loop.
| FileUtils.deleteDirectory(dir); | ||
| LOG.info(String.format("Deletion of directory %s was successful.", dir)); | ||
| break; | ||
| } catch (IOException ioException) { |
There was a problem hiding this comment.
File.walk().forEach() can throw UncheckedIOException. Probably good to catch that here too.
labkey-tchad
left a comment
labkey-tchad
left a comment
There was a problem hiding this comment.
I don't think we need to cast too wide of a net here. File locks on Windows are the only thing we've had trouble with (unless I'm forgetting something). We shouldn't add workarounds for hypothetical file permission issues that we don't understand.
| try { | ||
| String output = new String(p.getInputStream().readAllBytes(), StringUtilsLabKey.DEFAULT_CHARSET); | ||
| LOG.info("Running processes:\n" + output); | ||
| } |
There was a problem hiding this comment.
Consider try-with-resources for the stream
| try { | |
| String output = new String(p.getInputStream().readAllBytes(), StringUtilsLabKey.DEFAULT_CHARSET); | |
| LOG.info("Running processes:\n" + output); | |
| } | |
| try (InputStream is = p.getInputStream()) { | |
| String output = new String(is.readAllBytes(), StringUtilsLabKey.DEFAULT_CHARSET); | |
| LOG.info("Running processes:\n" + output); | |
| } |
| FileUtils.deleteDirectory(dir); | ||
| LOG.info(String.format("Deletion of directory %s was successful.", dir)); | ||
| break; | ||
| } catch (IOException ioException) { |
There was a problem hiding this comment.
We should just rethrow permissions exceptions. We haven't actually run into errors with those.
| } catch (IOException ioException) { | |
| } catch (AccessDeniedException adException) { | |
| throw adException; | |
| } catch (IOException ioException) { |
| } | ||
|
|
||
| } catch (IOException diagnosticException) { | ||
| LOG.warn("Failed to run lock diagnostic: " + diagnosticException.getMessage()); |
There was a problem hiding this comment.
Loggers can include the entire stack-trace and I think it would be useful to do so.
| LOG.warn("Failed to run lock diagnostic: " + diagnosticException.getMessage()); | |
| LOG.warn("Failed to run lock diagnostic: " + diagnosticException.getMessage(), diagnosticException); |
| dir.setWritable(true, false); | ||
|
|
||
| // Wrap in a try to close the stream. | ||
| try (Stream<Path> files = Files.walk(dir.toPath())) { | ||
| files.forEach(p -> p.toFile().setWritable(true, false)); | ||
| } |
There was a problem hiding this comment.
I don't think the tests should be in the business of changing file permissions (unless that's something we are explicitly testing at some point).
If the server (or other process) is leaving a file in an unwritable state, that is something we probably wouldn't want to ignore.
| } catch (IOException ioException) { | ||
| LOG.warn(String.format("IOException trying to delete directory %s. Error: %s. Waiting 10s and retrying. Attempt %d of 10.", | ||
| dir, ioException.getMessage(), attempt)); | ||
| if (attempt == 10) { |
There was a problem hiding this comment.
Put the max attempt count in a constant to ensure that this stays in sync with the for loop.
e.g.
| if (attempt == 10) { | |
| if (attempt == MAX_ATTEMPTS) { |
3 participants
Rationale
Revert AssayTransformMissingParentDirTest.testMissingParentDirectoryRegression to (mostly) its original state.
Move the delete code into TestFileUtils.deleteDirWithRetry, this might be useful to help debug failures in the future.
Related Pull Requests
Changes