Copacimportertest

[zesaro]

New PR on a new branch.
Edited Tests. No assignments. Better formatting. Cover DT (documenttype).
New Test in case an empty Text is imported.

Edit: Added the suggestions into the testfile. Checking the equality of Copac and RIS is ongoing.

[simonharrer]

Can you please have a look at https://ondemand.coverity.com/jobs/uj1ep5hu8118n66eqa61rs5ofc/results/f/4/8/9/620b152144b541f48db807c7eed9.html which indicates issues in the test class. Please close the resources with a try-with-resources block.

Regarding the testIsRecognizedFormat, I think it would be better if there are two lists, one for the accepted and one for the rejected files which can then be iterated and check in a for loop each. This would allow to add all the other files from the other importers that will trickle in over the next month.

[zesaro]

Am I assuming right that you mean something like that?

    @Test
    public void testIsRecognizedFormatAccept() throws IOException {
        CopacImporter importer = new CopacImporter();
            LinkedList<String> list = new LinkedList<>();
            list.add("CopacImporterTest1.txt");
            list.add("CopacImporterTest2.txt");

            for (String str : list) {
                try (InputStream is = CopacImporterTest.class.getResourceAsStream(str)) {
                    Assert.assertTrue(importer.isRecognizedFormat(is));
                    is.close();
                }
            }
    }

[lenhard]

Almost :) As you are using try-with-resources, the line is.close() is no longer needed. The try block takes care of that automatically.

[oscargus]

I read somewhere that it is still recommended (by many) to keep the close(), primarily for readability/understanding I guess. Now, I haven't followed it that carefully though...

[simonharrer]

Do you have a link for that @oscargus ?

[oscargus]

Maybe not. I tried to find it, but to no avail. I found something which might be what I referred to, but it didn't actually say exactly that. Rather, I found many examples without the close().

[simonharrer]

When comparing lists, it is better to do an assert that way:

Assert.assertEquals(Collections.emptyList(), entries);

Then, the message when the assertion fails is much better, as it contains the entries in the list using their toString method.

Example:

List<String> values = new LinkedList<>();
values.add("asdf");

Assert.assertEquals(Collections.emptyList(), values);

results in

Exception in thread "main" java.lang.AssertionError: expected:<[]> but was:<[asdf]>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotEquals(Assert.java:834)
    at org.junit.Assert.assertEquals(Assert.java:118)
    at org.junit.Assert.assertEquals(Assert.java:144)
    at Test.main(Test.java:13)

[zesaro]

Do you want me to only edit the testImportEntries3() ?
Or should I create two Lists on every testImport() and only make one Assert at the end? For example

    @Test
    public void testImportEntries() throws IOException {
        Globals.prefs.put("defaultEncoding", "UTF8");

        CopacImporter importer = new CopacImporter();

        List<BibtexEntry> entries = importer.importEntries(
                CopacImporterTest.class.getResourceAsStream("CopacImporterTest1.txt"), new OutputPrinterToNull());
        Assert.assertEquals(1, entries.size());
        BibtexEntry entry = entries.get(0);

        List<String> idealValue = new LinkedList<>();
        idealValue.add("The SIS project : software reuse with a natural language approach");
        idealValue.add("Prechelt, Lutz and Universität Karlsruhe. Fakultät für Informatik");
        idealValue.add("Interner Bericht ; Nr.2/92");
        idealValue.add("1992");
        idealValue.add("Karlsruhe :  Universitat Karlsruhe, Fakultat fur Informatik");
        idealValue.add("Edinburgh");
        idealValue.add("TXT");

        List<String> isValue = new LinkedList<>();
        isValue.add(entry.getField("title"));
        isValue.add(entry.getField("author"));
        isValue.add(entry.getField("series"));
        isValue.add(entry.getField("year"));
        isValue.add(entry.getField("publisher"));
        isValue.add(entry.getField("HL"));
        isValue.add(entry.getField("documenttype"));

        Assert.assertEquals(idealValue, isValue);

        Assert.assertEquals(BibtexEntryTypes.BOOK, entry.getType());
    }

[simonharrer]

I meant only when checking that the size of the list should be 0. You do not get too much information that the list has size 1. It is much easier to debug when you know what values the element in the list has.

[zesaro]

Okay. Now I know what you mean. Thank you.
After I have changed testImportEntries() to the "two list model" I got a failure. I couldn't resolve it because it was one big Message and I didn't saw my mistake.
This is why I asked before I changed the other one and pushed it.

[lenhard]

I just realized that a few fixes I implemented when going through coverty warnings this morning (see 5b66b98) conflict with this PR. I fixed the resource leaks and the remainder of this PR is still valuable.

@zellerdev To get it into master, please merge master into your branch. Sorry for the inconvenience, I'll keep my hands off the importers from now.

[koppor]

I noticed that line 53 reads

Globals.prefs.put("defaultEncoding", "UTF8");

Could you please change that string to UTF-8 and check if your test cases still work? - JabRef changed the strings to be more conforming to usual strings - see #155.

Furthermore, could you follow the pattern "If you modify preference, use following pattern:" stated at https://github.com/JabRef/jabref/wiki/Code-Howtos#test-cases?

[lenhard]

I'd like to add that you should not refer to preferences via their string key, but rather via their static variable in JabRefPreferences, e.g.:

Globals.prefs.put(JabRefPreferences.DEFAULT_ENCODING, "UTF-8");

[koppor]

I think, it's easier for reviewing if you squash the whole commits into one commit. git reset master is your friend.

[tobiasdiez]

I think it is slightly better to understand if you use @Test(expect=IOException.class) instead of the thrown construction.

[koppor]

I agree. In this case, no the whole method may throw an exception and not a single line (as IMHO discussed with @bartsch-dev in another PR)

[koppor]

Coverage 100%. 👍

While wondering why the import source is so small, I checked the header comment http://copac.jisc.ac.uk/faq/#format to check whether there are more fields. There, I found "How do I import Copac records in reference management software? (e.g. Endnote/Zotero)" which contains "If you use Reference Manager you can use the RIS filter to import Copac records."

@obraliar @mairdl @zellerdev @ayanai1 Could you please check whether RIS is really equal to Copac? Can you read each other's code and test cases and report back here?

In case they are equal, I would propose to merge the importer code.

[koppor]

@zellerdev I can't see what you changed why in your commit. In this case, it would have been better to add another commit. Then, I can easily see, what you changed after my approval. Now, I have to review the whole commit again and I don't know what I have been approved and what not.

[zesaro]

I apologize for that. Because of the problems building the project I had in the past this was a routine for me. I just included the suggestions you made on the top of the class.

[tobiasdiez]

What is HL? From the content, I would suspect it represents the location/city of the publisher. In this case it should be written to the address field in bibtex.

[zesaro]

This implementation of CopacImporter knows the tags TI, AU, PY, PU, SE, IS, KW, NT, PD and DT. In this case Copac doesn't know what HL is. So to address this field you have to use the tag HL as field. This ressources already existed. In this case this is a test to watch whether the importer correctly handles with unknown tags.

[stefan-kolb]

@koppor WDYT?

[koppor]

Please fix the formatting

[simonharrer]

Can you please rebase on current master?

[koppor]

Coverage 98,48%

[simonharrer]

Then provide a method in the BibtexEntryAssert class which can do this, please.

[zesaro]

Using java.nio for getTestFiles(), removed InspecSilverPlatter and replaced it with another Inspec file which is not recognized by copac and added a new method for BibtexEntryAssert using InputStreams and an ImportFormat as parameter to generate to lists to compare. I hope I solved everything as expected.

[simonharrer]

a directory stream is a resource which must be closed. use a try-with-resources construct here, please.