-
Notifications
You must be signed in to change notification settings - Fork 242
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
Additional unit tests for IOUtil #1149
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1149 +/- ##
==============================================
+ Coverage 68.156% 68.33% +0.174%
- Complexity 7962 8001 +39
==============================================
Files 540 541 +1
Lines 32622 32681 +59
Branches 5533 5536 +3
==============================================
+ Hits 22234 22331 +97
+ Misses 8169 8124 -45
- Partials 2219 2226 +7
|
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.
Welcome to htsjdk and thanks for submitting these tests.
@@ -520,4 +527,253 @@ public void testIsBlockCompressedOnJimfs(Path file, boolean checkExtension, bool | |||
Assert.assertEquals(IOUtil.isBlockCompressed(jimfsFile, checkExtension), expected); | |||
} | |||
} | |||
@DataProvider | |||
public static Object [][] filesToCompress(){ | |||
return new Object[][]{ |
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.
IntelliJ has a "Reformat Code" option which will clean up a lot of the stylistic issues that I might list here:
- fewer empty lines
- space between ']' and '('
- space after ','
- space between ')' and '{'
- no newline between ')' and '{'
- space around operators
- space between if/while/for and '('
- no space between method-name and '('
- if/else with brackets and the else line looking like '} else {'
-make final any variable that can be (method arguments included)
public void testCompressionLevel(Path file,String extension,int lastDifference) throws IOException{ | ||
long origSize=file.toFile().length(); | ||
long previousSize=origSize; | ||
String tmpPath = System.getProperty("java.io.tmpdir"); |
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.
use File.createTempFile for creating temporary files. (here and below)
File tmpDir = new File(tmpPath, userName); | ||
tmpDir.mkdir(); | ||
tmpDir.deleteOnExit(); | ||
for(int cl=1;cl<=9;cl++) |
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.
use meaningful variable names
OutputStream outStream=IOUtil.openFileForWriting(outFile.toFile()); | ||
IOUtil.transferByStream(inStream,outStream,origSize); | ||
outStream.close(); | ||
outFile.toFile().deleteOnExit(); |
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.
put deleteOnExit() call right after file "creation"
InputStream inStream=IOUtil.openFileForReading(file); | ||
OutputStream outStream=IOUtil.openFileForWriting(outFile.toFile()); | ||
IOUtil.transferByStream(inStream,outStream,origSize); | ||
outStream.close(); |
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.
for resources that need to be closed, use try-with-resources
IOUtil.transferByStream(inStream,outStream,origSize); | ||
outStream.close(); | ||
outFile.toFile().deleteOnExit(); | ||
long newSize=outFile.toFile().length(); |
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.
use Files.size(Path)
|
||
} | ||
|
||
@Test(dataProvider = "badCompressionLevels") |
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.
use "expectedException" on the test here.
tmpDir.mkdir(); | ||
tmpDir.deleteOnExit(); | ||
Path outFile=Paths.get(tmpPath,userName,"tmp"+file.getFileName()); | ||
ProcessExecutor.execute(new String[]{"touch", outFile.toAbsolutePath().toString()}); |
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.
I think it would be better to open and close the file than to spawn an external command here.
tmpDir.mkdir(); | ||
tmpDir.deleteOnExit(); | ||
Path file = Paths.get(tmpDir.getAbsolutePath(), "tmp.txt"); | ||
Random rand = new Random(System.currentTimeMillis()); |
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.
Better to have no true randomness in code...use a fixed seed here.
Thanks Yossi. I have updated to address your requests. |
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.
looking better!
some more comments.
{TEST_DATA_DIR.resolve("words_longs.txt"), ".bfq", 8}, | ||
{TEST_VARIANT_DIR.resolve("test1.vcf"), ".gz", 7}, | ||
{TEST_VARIANT_DIR.resolve("test1.vcf"), ".bfq", 7} | ||
|
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.
still too many newlines here and other places.
IOUtil.copyFile(file.toFile(), outFile.toFile()); | ||
|
||
|
||
final Stream<String> stream = Files.lines(file); |
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.
given that we do not need to save on memory here,
you can replace the remaining lines of code here with a
Assert.assertEquals(File.lines(file),Files.lines(outFile))
try { | ||
IOUtil.copyFile(file.toFile(), outFile.toFile()); | ||
} catch (SAMException e) { | ||
file.toFile().setReadable(true); |
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.
why change the readability status here?
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.
if this is needed then put a comment.
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.
you want a finally here not a catch.
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.
yup, you're right should be a finally. For reference, need to change back to readable so that other tests which need to read this file later will not fail (along with just being best to leave things as they were before the test). adding explanation as comment in code.
if (listExpected.contains(name)) { | ||
expectedFiles.add(file.toFile()); | ||
} | ||
file.toFile().deleteOnExit(); |
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.
move this up to be right after the resolve
call.
writer.newLine(); | ||
} | ||
} | ||
final IterableOnceIterator<String> retLines = IOUtil.readLines(file.toFile()); |
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.
again, this whole block can be replaced with a single assert, I think.
final Stream<Path> fileStream = Files.walk(TEST_VARIANT_DIR); | ||
final Stream<Path> copyFileStream = Files.walk(copyToDir); | ||
|
||
final Iterator<Path> itFileStream = fileStream.iterator(); |
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.
inline stream and iterator
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.
you can simplify this by actually using the stream...
final List<Path> collect = Files.walk(TEST_VARIANT_DIR).filter(f -> !f.equals(TEST_VARIANT_DIR)).collect(Collectors.toList());
and then compare with Assert.assertEquals()
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.
A 2mb file of random words is a very large test file for htsjdk. Would it be possible to reduce he file size? One easy thing to do would be to zip it in the repo and unzip it for the test. Another thing might be to generate it from a smaller underlying dictionary instead.
…emporary long word file for compression testing in IOUtilTest
Sure. I went the dictionary route, so the 2mb random word file is replaced with a ~50kb dictionary which is used to create a temporary few mb random word file (which gets deleted on exit) instead. |
@kachulis Thanks. I appreciate the extra effort. One 2mb isn't that big of a deal, but if we add a lot of them we end up with a huge unwieldy repo so it's always good to keep things slim. |
@lbergelson no problem, that makes sense |
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.
A few last comments.
This is looking good! thanks!
//build long file of random words for compression testing | ||
WORDS_LONG = Files.createTempFile("words_long", ".txt"); | ||
WORDS_LONG.toFile().deleteOnExit(); | ||
List<String> wordsList = new ArrayList<String>(); |
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.
can be final
{-1}, | ||
{10} | ||
}; | ||
|
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.
straggler unneeded newline.
final Random rand = new Random(seed); | ||
final int nLines = 5; | ||
final List<String> lines = new ArrayList<String>(); | ||
try (final BufferedWriter writer = Files.newBufferedWriter(file)) { |
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.
Not sure it's nicer, but you can wrap the bufferedWriter with a PrintWriter and then call println() instead of write() + newLine().
final Path copyToDir = Files.createTempDirectory("copyToDir"); | ||
copyToDir.toFile().deleteOnExit(); | ||
IOUtil.copyDirectoryTree(TEST_VARIANT_DIR.toFile(), copyToDir.toFile()); | ||
final List<String> collect = Files.walk(TEST_VARIANT_DIR).filter(f -> !f.equals(TEST_VARIANT_DIR)).map(p -> p.getFileName().toString()).collect(Collectors.toList()); |
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.
is toString needed after getFileName?
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.
No, you're right, can make this a List<Path>
instead of a List<String>
and still works.
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.
I updated your comment since github sanitizes things that look like html....
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.
ha, oops, thanks
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.
Thanks!
Description
Additional unit test to IOUtilTest to increase coverage of IOUtil.
Checklist