-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
feat(test): cowtowncoder#87 added a new test for LockedFile #110
feat(test): cowtowncoder#87 added a new test for LockedFile #110
Conversation
- A cool feature that I discovered is that the constructor will create files for you even if they do not exist - It looks like it has some hidden logic for negative timestamps
public void constructor_givenNonExistentFile_shouldCreateANewFile() throws IOException { | ||
// given | ||
File blankFile = temporaryFolder.newFile(); | ||
File nonExistentFile = new File(blankFile + ".nonexistent"); | ||
|
||
if (Files.exists(nonExistentFile.toPath())) { | ||
fail("temp file should not exist"); | ||
} | ||
|
||
// when | ||
new LockedFile(nonExistentFile); | ||
|
||
// then - the nonexistent file now exists? | ||
assertTrue(Files.exists(nonExistentFile.toPath())); | ||
assertTrue(nonExistentFile.canRead()); | ||
assertTrue(nonExistentFile.canWrite()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the really cool use case where if you give it a nonexistent file, the LockedFile class will go ahead and create the file. Pretty neat ey?
public class LockedFileTest | ||
{ | ||
@Rule | ||
public TemporaryFolder temporaryFolder = new TemporaryFolder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take note that I do leverage the TemporaryFolder library to create fake dummy files that are automatically cleaned up after the test finishes up.
@Test | ||
public void constructor_givenNull_shouldThrowNullPointerException() throws IOException { | ||
try { | ||
new LockedFile(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, this is kind of unit test that I think is basically worthless, and I don't want to add too many similar even for compliance purposes.
I'll tweak the PR and leave just skeletal aspect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
First of all, thank you for submitting this! While I am not a big fan of super-extensive testing for purposes of code coverage, it is cool to get some coverage for this helper class. So that's good. On
yeah, in hindsight I think that is slightly wrong (my fault obviously :) ) -- constructors should not typically do such heavy lifting. But I wrote JUG 20 years ago so... live & learn :) |
@SquireOfSoftware well done! Code coverage (line coverage, I think?) shot up by 5% pct units, now well over 50% overall. |
What did I do?
As per this comment: #87 (comment), I wrote some tests for LockedFile covering:
Extra notes
There were also some cool things that I discovered:
Let me know if the coverage is not good enough and I can add more usecases to the tests