Stop stashing tmp directories for tests #21412
Conversation
github-actions
bot
added
team-Local-Exec
| } | ||
| path.getChild("execroot").renameTo(stashPathExecroot); | ||
| if (isTestAction(mnemonic)) { | ||
| // We only do this after the rename operation has succeeded |
There was a problem hiding this comment.
Not related to a change made in this PR, but the usage of synchronized in line 136 looks unnecessary.
| stashPath.createDirectory(); | ||
| Path stashPathExecroot = stashPath.getChild("execroot"); | ||
| if (isTestAction(mnemonic)) { | ||
| treeDeleter.deleteTree(path.getRelative("execroot/" + environment.get("TEST_WORKSPACE") + "/_tmp")); |
There was a problem hiding this comment.
This makes it so that the containing method may now perform relatively costly IO. Is this clearly fine for the callers, for example because they would clean out the entire sandbox if it weren't reused?
There was a problem hiding this comment.
As far as I can tell this would indeed get more expensive for the caller here when async deletion is disabled (when it's enabled, there's no significant cost). It also doesn't mean that sync deletion had no cost before this change, it was just paid somewhere else when cleaning up that directory during reuse.
I don't think this matters much but please correct me if I'm wrong.
|
Thank you Fabian. |
github-actions
bot
removed
the
awaiting-review
Test actions create a `_tmp` directory that doesn't make sense to stash between different test actions when using `--reuse_sandbox_directories`. This change makes sure that doesn't happen by deleting the directory before stashing. Closes #21412. Fixes #21253. PiperOrigin-RevId: 608566281 Change-Id: I7186f8e5eba7b3110b10963df69ec66cd1402b2c
2 participants
Test actions create a
_tmpdirectory that doesn't make sense to stash between different test actions when using--reuse_sandbox_directories. This change makes sure that doesn't happen by deleting the directory before stashing.