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

Cleanup FindFile and asssociated tests #1596

Merged
merged 12 commits into from
Jul 25, 2016
48 changes: 26 additions & 22 deletions src/main/java/net/sf/jabref/logic/util/io/FileFinder.java
Original file line number Diff line number Diff line change
@@ -1,43 +1,47 @@
package net.sf.jabref.logic.util.io;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.BasicFileAttributes;
import java.util.Collection;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import java.util.function.BiPredicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

public class FileFinder {

private static final Log LOGGER = LogFactory.getLog(FileFinder.class);


public static Set<File> findFiles(Collection<String> extensions, Collection<File> directories) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just use List here instead of Collection?

Set<File> result = new HashSet<>();

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there cases where the result is explicitly checked for an empty Set and some warning related to permissions is shown?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I misread the code... It should be OK.

for (File directory : directories) {
result.addAll(FileFinder.findFiles(extensions, directory));
}
Objects.requireNonNull(directories, "Directories must not be null!");
Objects.requireNonNull(extensions, "Extensions must not be null!");

return result;
}
BiPredicate<Path, BasicFileAttributes> isDirectoryAndContainsExtension = (path,
attr) -> !Files.isDirectory(path)
&& extensions.contains(FileUtil.getFileExtension(path.toFile()).orElse(""));

private static Collection<? extends File> findFiles(Collection<String> extensions, File directory) {
Set<File> result = new HashSet<>();
for (File directory : directories) {

File[] children = directory.listFiles();
if (children == null) {
return result; // No permission?
}
try (Stream<File> files = Files.find(directory.toPath(), Integer.MAX_VALUE, isDirectoryAndContainsExtension)
.map(x -> x.toFile())) {
result.addAll(files.collect(Collectors.toSet()));

for (File child : children) {
if (child.isDirectory()) {
result.addAll(FileFinder.findFiles(extensions, child));
} else {
FileUtil.getFileExtension(child).ifPresent(extension -> {
if (extensions.contains(extension)) {
result.add(child);
}
});
} catch (IOException e) {
LOGGER.error("Problem in finding files", e);
}
}

return result;
}

}
}
116 changes: 0 additions & 116 deletions src/test/java/net/sf/jabref/FileBasedTestCase.java

This file was deleted.

78 changes: 0 additions & 78 deletions src/test/java/net/sf/jabref/FileBasedTestHelper.java

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public void movesFileFromSubfolder() throws IOException {
public void movesFileFromSubfolderMultiple() throws IOException {
File subfolder = bibFolder.newFolder();
File fileBefore = new File(subfolder, "test.pdf");
assertTrue(fileBefore.createNewFile());;
assertTrue(fileBefore.createNewFile());
assertTrue(fileBefore.exists());

BibEntry entry = new BibEntry();
Expand Down
124 changes: 124 additions & 0 deletions src/test/java/net/sf/jabref/logic/util/io/FileBasedTestCase.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
package net.sf.jabref.logic.util.io;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Stream;

import net.sf.jabref.BibtexTestData;
import net.sf.jabref.Globals;
import net.sf.jabref.model.database.BibDatabase;
import net.sf.jabref.model.entry.BibEntry;
import net.sf.jabref.preferences.JabRefPreferences;
import net.sf.jabref.support.DevEnvironment;

import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assume.assumeFalse;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class FileBasedTestCase {

private BibEntry entry;
private Path rootDir;


@Before
public void setUp() throws IOException {
Globals.prefs = mock(JabRefPreferences.class);

BibDatabase database = BibtexTestData.getBibtexDatabase();
entry = database.getEntries().iterator().next();

rootDir = Files.createTempDirectory("UtilFindFileTest");
Copy link
Member

Choose a reason for hiding this comment

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

I think the proper way to handle temporary folders in tests is via
@Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();,
which also takes care of deleting the folder after every test.




Path subDir = Files.createDirectory(rootDir.resolve("Organization Science"));
Path pdfSubDir = Files.createDirectory(rootDir.resolve("pdfs"));

assertTrue(Files.exists(Files.createFile(subDir.resolve("HipKro03 - Hello.pdf"))));
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a smiple Files.createFile without the assertion for readability (and since I suspect that creating the file never fails...)

assertTrue(Files.exists(Files.createFile(rootDir.resolve("HipKro03 - Hello.pdf"))));

Path pdfSubSubDir = Files.createDirectory(pdfSubDir.resolve("sub"));
assertTrue(Files.exists(Files.createFile(pdfSubSubDir.resolve("HipKro03-sub.pdf"))));

Files.createDirectory(rootDir.resolve("2002"));
Path dir2003 = Files.createDirectory(rootDir.resolve("2003"));
assertTrue(Files.exists(Files.createFile(dir2003.resolve("Paper by HipKro03.pdf"))));

Path dirTest = Files.createDirectory(rootDir.resolve("test"));
assertTrue(Files.exists(Files.createFile(dirTest.resolve(".TEST"))));
assertTrue(Files.exists(Files.createFile(dirTest.resolve("TEST["))));
assertTrue(Files.exists(Files.createFile(dirTest.resolve("TE.ST"))));
assertTrue(Files.exists(Files.createFile(dirTest.resolve("foo.dat"))));

Path graphicsDir = Files.createDirectory(rootDir.resolve("graphicsDir"));
Path graphicsSubDir = Files.createDirectories(graphicsDir.resolve("subDir"));

assertTrue(Files.exists(Files.createFile(graphicsSubDir.resolve("HipKro03test.jpg"))));
assertTrue(Files.exists(Files.createFile(graphicsSubDir.resolve("HipKro03test.png"))));

}

@Test
public void testFindAssociatedFiles() {

List<BibEntry> entries = Collections.singletonList(entry);
List<String> extensions = Arrays.asList("jpg", "pdf");
List<File> dirs = Arrays.asList(rootDir.resolve("graphicsDir").toFile(), rootDir.resolve("pdfs").toFile());

when(Globals.prefs.getBoolean(JabRefPreferences.AUTOLINK_EXACT_KEY_ONLY)).thenReturn(false);
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can see, there is no need to mock Globals.prefs. A simple preferences = mock(...) and findAssociatedFiles(..., preferences) should work.

Is it much work to refactor findAssociatedFiles to accept a boolean AUTOLINK_EXACT_KEY_ONLY instead of the preference class?


Map<BibEntry, List<File>> results = FileUtil.findAssociatedFiles(entries, extensions, dirs, Globals.prefs);

assertEquals(2, results.get(entry).size());
Copy link
Member

Choose a reason for hiding this comment

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

Better create a Map<BibEntry, List<File>> expected and compare that it equals results

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case this is not the best idea, as the pahts to the file are os dependent resolved and differ on the location of the temp file etc.

assertTrue(results.get(entry)
.contains(rootDir.resolve(Paths.get("graphicsDir", "subDir", "HipKro03test.jpg")).toFile()));
assertFalse(results.get(entry)
.contains(rootDir.resolve(Paths.get("graphicsDir", "subDir", "HipKro03test.png")).toFile()));
assertTrue(results.get(entry).contains(rootDir.resolve(Paths.get("pdfs", "sub", "HipKro03-sub.pdf")).toFile()));
}

@Test
public void testFindFilesException() {
List<String> extensions = Arrays.asList("jpg", "pdf");
List<File> dirs = Arrays.asList(rootDir.resolve("asdfasdf/asdfasdf").toFile());
Set<File> results = FileFinder.findFiles(extensions, dirs);

assertEquals(0, results.size());
Copy link
Member

@tobiasdiez tobiasdiez Jul 23, 2016

Choose a reason for hiding this comment

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

compare results to emptyset

}

@Ignore("Fails on CI Server")
@Test(expected = NullPointerException.class)
public void testFindFilesNullPointerException() {

assumeFalse(DevEnvironment.isCIServer());
FileFinder.findFiles(null, null);
}

@After
public void tearDown() throws IOException {

Globals.prefs = null;
Copy link
Member

Choose a reason for hiding this comment

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

the Globals.prefs = null is not required.

try (Stream<Path> path = Files.walk(rootDir)) {
// reverse; files before dirs
for (Path p : path.sorted((a, b) -> b.compareTo(a)).toArray(Path[]::new)) {
Files.delete(p);
}
}
}
}
Loading