Medline plain importer test

[boceckts]

Fixed IndexOutOfBounds for MedlinePlainImporter when Testfile has lines with too few characters. Removed unreachable code. Created 100% Testcoverage for MedlinePlainImporter.

[simonharrer]

Please use rebase --interactive to squash the commits with messages like "" or see last commit.

[simonharrer]

Please see #310 for how to handle tests for this part.

[koppor]

@boceckts java.lang.StringIndexOutOfBoundsException: String index out of range: -1 during test. Please fix 😇

[koppor]

@boceckts Please squash the whole branch history into one commit. The magic command is git rebase master. See #379 (comment) for a longer discussion.

[boceckts]

Commits sind nun zu einem Commit zusammengefasst, index out of bounds exception gefixt und die Testmethoden sind nun leicht abgeändert zu AssertEquals.

[koppor]

Commit looks good 👍, but the test fail. Could you please investigate?

[boceckts]

Welche Tests genau? Wenn ich die Klasse MedlinePlainImporterTest.java als JUnit Test ausführe, laufen alle Test bei mir ohne Fehler..

[simonharrer]

!contains is better readable then indexOf, I think.

[koppor]

Siehst du hier beim Pull-Request die Meldung "All checks have failed"? Screenshot:

Du kannst dann auf "Details" bei "ci/circleci" klicken. Dort steht dann "junit failures: testIsRecognizedFormat - in net.sf.jabref.importer.fileformat.MedlinePlainImporterTest". Hier kannst du auf "more info" klicken. Da steht dann

java.lang.NullPointerException
    at java.io.Reader.<init>(Reader.java:78)
    at java.io.InputStreamReader.<init>(InputStreamReader.java:97)
    at net.sf.jabref.importer.ImportFormatReader.getReaderDefaultEncoding(ImportFormatReader.java:352)
    at net.sf.jabref.importer.fileformat.MedlinePlainImporter.isRecognizedFormat(MedlinePlainImporter.java:69)
    at net.sf.jabref.importer.fileformat.MedlinePlainImporterTest.testIsRecognizedFormat(MedlinePlainImporterTest.java:50)

[boceckts]

@simonharrer changed it to !contains.
@koppor Seems like one character in the name of the test file was a different case than what I wrote in the java test class.

[koppor]

Your test file looks good. Seems to cover seven different entry types with different fields and different settings (doi, multine line titles). However, your test only treats two of them. Why don't you check all test entries?

You could check the result via prepared bibtex files. See https://github.com/JabRef/jabref/blob/6293b4d8ee9409afb25b402643a31904b56e33a0/src/test/java/net/sf/jabref/importer/fetcher/GVKParserTest.java. We are trying to get #378 through so that others can use these test improvements, too.

[koppor]

@boceckts We are ready to go from our side. Please have a look at GVKParserTest.java.

[boceckts]

The Unit Test failes because multiple comments are being imported with a newline separator. However in the bibtex source the new lines are being ommitted therefore the assertion fails when testing with a import file using multiple comments such as "MedlinePlainImporterTest2.txt vs MedlinePlainImporterTestBib2.bib".

[simonharrer]

Always use try-with-resources for getResourceAsStream usages.

[tobiasdiez]

Split tests. Definitely a separate test for recognized and one for not-recognized files. I would even say that there should be a separate test for every file (maybe not for all files, just the ones which actually test a new behavior...say one for txt files and one for isi).

[boceckts]

Split tests is a great idea. I think one test method for isRecognized and one for isNotRecognized should be enough since this methods should not test any data but only check if the format is recognized or not.

[tobiasdiez]

Yes, you are right, normally one should be enough. But have a look at the isRecognized method and see if you cover all cases. Also it does not harm to check edge cases, like empty files.

[tobiasdiez]

Good work 👍 .
Some tests are hard to read since they compare a medline import with a bib import. So it may be hard to see why they fail.

Better test specific features in small test cases (and maybe leave one big "integration" test). For example, (using pseudo-code)

public importParsesDoi() {
  Stream stream = StreamFromString("PMID-22664220" + "\n" + "AID - doi:10.1016/j.cpr.2005.02.002");
  List<Entries> entries = Import(stream);

  BibEntry expected = new BibEntry();
  expected.setField("doi", "10.1016/j.cpr.2005.02.002"); 
  Assert(entries = Collections.singletonList(expected));
}

could replace the test 4. See also https://github.com/JabRef/jabref/wiki/Code-Howtos#test-cases and http://stackoverflow.com/questions/782178/how-do-i-convert-a-string-to-an-inputstream-in-java.

[koppor]

Your test fail, because equals is not working on the canonical representation. We are afraid that changing equals (and hashCode) leads to unwanted side effects.

Therefore, please add a method to BibtexEntryAssert for comparing Lists (please use the List interface, not the ArrayList implementation). There, you can use assertEquals(BibEntry shouldBeEntry, BibEntry entry) for comparing the entries themselves.

[koppor]

Please also try to get rid of the warnings by eclipse. I think try-with-resources is the solution, isn't it?

[boceckts]

#579

[boceckts]

Added more test methodes to cover more if conditions and included the suggestions of the comments.

[tobiasdiez]

Only the last (positive) assert statement is necessary.

[tobiasdiez]

Looks very good to me! I just pointed out some smaller things. If they are fixed, this can be merged in (from my point of view).

[boceckts]

Thank you for the comments, I removed the unnecessary NotEquals and added tests for the remaining entries in the test file.

[koppor]

Coverage 98.65%. I assume that covering line 126 was difficult, because there is no entry where the fourth character of the second line is NOT -. Or similar 😃