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

feat(test): cowtowncoder#87 added a new test for LockedFile #110

Conversation

SquireOfSoftware
Copy link
Contributor

@SquireOfSoftware SquireOfSoftware commented Jun 7, 2024

What did I do?

As per this comment: #87 (comment), I wrote some tests for LockedFile covering:

  • The constructor - I didn't test doDeactivate or deactivate as I figured the constructor would test...most of the code paths here
  • The readStamp
  • The writeStamp

Extra notes

There were also some cool things that I discovered:

  • 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
  • It looks like there is Junit4 that I could leverage 🎉

Let me know if the coverage is not good enough and I can add more usecases to the tests

- 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
Comment on lines +48 to +64
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());
}
Copy link
Contributor Author

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();
Copy link
Contributor Author

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);
Copy link
Owner

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.

Copy link
Owner

@cowtowncoder cowtowncoder left a comment

Choose a reason for hiding this comment

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

LGTM

@cowtowncoder
Copy link
Owner

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.
I only added one quick note on things I do not really want spent any time on (argument validation for internal helper classes).

On

A cool feature that I discovered is that the constructor will create files for you even if they do not exist

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 :)
(it's just a matter of keeping such logic in factory method, IMHO, nothing more special; constructors should only contain argument validation, assignments, nothing else)

@cowtowncoder cowtowncoder merged commit 64297d0 into cowtowncoder:master Jun 7, 2024
4 checks passed
@cowtowncoder
Copy link
Owner

@SquireOfSoftware well done! Code coverage (line coverage, I think?) shot up by 5% pct units, now well over 50% overall.

@SquireOfSoftware SquireOfSoftware deleted the add_some_tests_to_lockedfile branch June 8, 2024 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants