Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Randomize naming for createVolumeDirectory method #4195

Merged
merged 3 commits into from
Sep 23, 2021

Conversation

johnathana
Copy link
Contributor

Currently createVolumeDirectory method uses System.currentTimeMillis() to randomize the volume directory name. This appears to be problematic for test suites running in parallel, falling to the exact same millisecond causing random test failures.

Copy link
Member

@rnorth rnorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @johnathana, this seems like a sensible fix to apply

@bsideup
Copy link
Member

bsideup commented Jun 10, 2021

@rnorth btw we should mark it as deprecated as the method is unused

@johnathana
Copy link
Contributor Author

@rnorth btw we should mark it as deprecated as the method is unused

This is a nice utility function. I use it to extract jar classhpath resources in CI docker in docker environments.

@bsideup
Copy link
Member

bsideup commented Jun 10, 2021

@johnathana consider using MountableFile abstraction for that :)

@johnathana
Copy link
Contributor Author

@johnathana consider using MountableFile abstraction for that :)

Thank you for your prompt response @bsideup. I am already using MountableFile and the getResolvedPath method to do all the heavy lifting, extracting to a temp dir. In CI envs and specifically Jenkins, docker in docker can be tricky. So I want files to the CWD. The final code looks like that:

def extractClasspathResourceFolderToCwd(resourceFolder: String): String = {
  val resourcePath      = MountableFile.forClasspathResource(resourceFolder).getResolvedPath
  val destinationFolder = createVolumeDirectory(true)
  FileUtils.copyDirectory(new File(resourcePath), destinationFolder.toFile)
  destinationFolder.toAbsolutePath.toString
}

@bsideup
Copy link
Member

bsideup commented Jun 10, 2021

@johnathana since creating temporary files in a current directory isn't Testcontainers' responsibility, I'd still prefer to remote that unused method, especially given how simple it is.

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix and adding the deprecation notice @johnathana.

@kiview kiview added this to the next milestone Sep 23, 2021
@kiview kiview merged commit b88d136 into testcontainers:master Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants