junit-team / junit4

A programmer-oriented testing framework for Java.
https://junit.org/junit4
Eclipse Public License 1.0
8.53k stars 3.29k forks source link

TemporaryFolder default name generation inconsistent #1714

Closed conbon closed 3 years ago

conbon commented 3 years ago

TemporaryFolder relies on java.io.File$TempDirectory.

The generateFile method uses SecureRandom to generate a long to ascribe to the file name.

This can cause varying lengths of temporary folder name.

filePath: /tmp/junit2375994787548427572/test.txt --> long is 19 chars filePath: /tmp/junit3617134405501405307/test.txt --> long is 19 chars again filePath: /tmp/junit343221902873074958/test.txt --> long is 18 chars

This was discovered during serialisation of path names and byte length checks in tests causing intermittent issues.

A switch to using a UUID.randomUuid() and actually implementing temp folder creation in junit might be a way to fix this (I don't see an api on java.io.File which allows passing the random part of the filename).

junit: 4.12 os: ubuntu 20.04

sbrannen commented 3 years ago

A switch to using a UUID.randomUuid() and actually implementing temp folder creation in junit might be a way to fix this

Why would the JUnit team want to "fix" this, since it's not technically broken?

You get a temporary directory, which is what the feature offers.

conbon commented 3 years ago

Not technically broken, no. Fix may have been a bad word to use.

Although it might be nice to provide an extension to the temporary folder class to allow an non-varying length folder name to be created.

It's just that it introduces a kind of randomness I wouldn't want to see in a test library. Maybe that was just an oversight of using a handy looking API in the File class at the time.

Would PRs be accepted in this area?

kcooney commented 3 years ago

TemporaryFolder used to have its own implementation for creating the directory. We intentionally moved away from that in the recent release of JUnit. I'm not sure this is a compelling reason to revisit that decision.

kcooney commented 3 years ago

(sorry didn't mean to close this quite yet)

marcphilipp commented 3 years ago

I also don't see this as a compelling reason to revisit that decision.

conbon commented 3 years ago

OK to close this then. Thanks for the fast response.