Reenable more tests #2371

stefan-kolb · 2016-12-12T15:53:50Z

Corrections of wrong Bibtex serializations: I inverted the tests for the file field so they test if the fields are not changed for \n and \t.
Edge case behavior: Not implemented right now and won't be as @lenhard says. Removed for now until implemented.
Author parsing
The result is: Expected :Other, A. N.; Actual :Other, Jr., A. N. We decided that this is expected behavior for now.
Layouter test: logic is just not handling \n\n to <br> conversion. Cannot be handled anymore as the BibTeX parser strips duplicated whitespaces by default!
XMP test: Fixed
OO test: Fixed
Ignored GUI tests: Will stay ignored for now.

stefan-kolb · 2016-12-12T15:59:58Z

@lenhard You can probably comment a little bit on the test that were previously inside the parser.

stefan-kolb · 2016-12-12T16:09:28Z

I tend to either fix the ignored tests or remove them. For some of them I don't even know what they should do...

stefan-kolb · 2016-12-12T17:20:33Z

@JabRef/developers I need help from all of you that have expertise in at least one of these topics!

tobiasdiez · 2016-12-12T18:54:21Z

The tests for the file field are a bit strange. I'm not sure why tabs and newlines are bad there. But, anyways, the parser shouldn't remove whitespace; this should be done in a separate cleanup.

The result of the author parsing also looks good to me (although one could argue that the Jr part is unimportant and should be deleted in an abbreviated representation...).

koppor · 2016-12-12T21:51:23Z

XMP: This is JabRef's implemenation to store BibTeX data inside PDFs to ease sharing of BibTeX data along with the PDF. Refs #938 #1096 (comment). Think, we have to switch to Dublin Core or to biblatexml. This takes more than a few hours. Since I am fan of this way of sharing, I'd like to keep the tests to be able to think of backward compatibility.

lenhard · 2016-12-13T09:11:45Z

My 2 cents:

newline / tab elimination: That happens in FieldContentParser, which is called during parsing, and does work. However, the file field is explicitly exempted from this behavior, so the test fails (trying to fix the content of the file field is calling for problems). I think this should be changed to some other field, say title and the tests would be fine.
edge case: This is documented in Saved file cannot be read #1212 and does not work. With the current structure of the parser, this is also far from trivial, since it would need a lot of look-ahead in the parsing. My strategy has been to ignore it until some users actually complain about it. I would keep on doing that.
author parsing: No idea really, I would keep ignoring it.
The html stuff I am not sure as well. I haven't really touched those formatters so far.

stefan-kolb · 2016-12-13T09:14:54Z

@lenhard Thanks for your input. So I just need to change the first tests to another field right now, e.g. title?!
I will remove the dge case then for now.
Looks like the author parser is correct.
So I'm just missing a resolution for the HTML and XMP stuff 😄 👍

matthiasgeiger · 2016-12-13T09:20:07Z

However, the file field is explicitly exempted from this behavior, so the test fails (trying to fix the content of the file field is calling for problems). I think this should be changed to some other field, say title and the tests would be fine.

Or: Rewrite the tests to check that the content of the file field is NOT changed. @stefan-kolb You should check whether tests for these two aspects are already in place - if so, you can just delete the ignored ones

lenhard · 2016-12-13T09:40:18Z

@stefan-kolb @matthiasgeiger If you invert the tests, then use either review or abstract. These are the only hard-coded fields where we avoid newline elimination. The other ones can be configured in the preferences and the file field is a default. But if a user really wants to, he can remove it from that list.

stefan-kolb · 2016-12-13T10:21:34Z

Ok, I fixed or removed all tests now. Only one ignored test is remianing in logic, which will be fixed by @matthiasgeiger in his custom entries PR?! The GUI tests still remain ignored.

…rips duplicated whitespaces

lenhard

One minor thing (formatting) and one request for clarification.

lenhard · 2016-12-13T14:03:14Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

    public void testCommandLineByKey() throws IOException, TransformerException {

        File tempBib = File.createTempFile("JabRef", ".bib");
        try (BufferedWriter fileWriter = Files.newBufferedWriter(tempBib.toPath(), StandardCharsets.UTF_8)) {
            fileWriter.write(t1BibtexString());
            fileWriter.write(t2BibtexString());
+            fileWriter.close();


Why do you need the close statement here? fileWrite is opened in a try-with-resources, so it will be closed at the end.

The test fails if I do not close the current fileWriter. Maybe it would be betetr to extract the code below from the try statement...

lenhard · 2016-12-13T14:04:01Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

    public void testCommandLineByKey() throws IOException, TransformerException {

        File tempBib = File.createTempFile("JabRef", ".bib");
        try (BufferedWriter fileWriter = Files.newBufferedWriter(tempBib.toPath(), StandardCharsets.UTF_8)) {
            fileWriter.write(t1BibtexString());
            fileWriter.write(t2BibtexString());
+            fileWriter.close();

            { // First try canh05


Not your change, but why is this bracket opening in a new line? Can you fix the formatting in this method?

The bracket below? This is some old stuff. I already forgot what this does in Java...creates a new isolated scope or so...

stefan-kolb · 2016-12-13T14:14:50Z

@lenhard Thanks for the review, some good input here!

simonharrer · 2016-12-13T14:23:25Z

src/main/java/net/sf/jabref/logic/openoffice/OOBibStyle.java

        this.prefs = Objects.requireNonNull(prefs);
        this.encoding = StandardCharsets.UTF_8;
        setDefaultProperties();
-        initialize(OOBibStyle.class.getResource(resourcePath).openStream());
+        initialize(OOBibStyle.class.getResourceAsStream(resourcePath));


be aware that the inputstream is null if the resource cannot be found, resulting in a NPE.

Is this any different than before?

Maybe we can wrap it inside Objects.requireNonNull() and also add some Objects.requireNonNull for resourcePath and prefs before lin 153

Added some requireNonNulls.

simonharrer · 2016-12-13T14:25:41Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

+            try {
+                XMPUtilMain.main(new String[]{"OezbekC06", tempBib.getAbsolutePath(), pdfFile.getAbsolutePath()});
+            } finally {
+                System.setOut(sysOut);
            }
        }


For something like this, I would prefer to create a utility method that can be called. Should be useful on other occasions as well. You can pass in a lambda function which will be called from the utility method, and the method could simply return the value of the sysout as a String.

To be honest, I didn't want to refactor the tests but just get them running again.

OK for leave as is - I copied @simonharrer's comment to JabRef#6.

simonharrer · 2016-12-13T14:26:06Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

+        if (!tempBib.delete()) {
+            // TODO: file is still locked
+            System.err.println("Cannot delete temporary file");
+        }


This should be either an assertion or deleted.

For some reason the file cannot be deleted and is still locked but I dunno where yet (and don't really care).

Why not just calling deleteOnExit()? This would prevent these kind of issues.

A better way to use a TemporaryFolder rule and create the file there. Then junit keeps track of deleting it

simonharrer · 2016-12-13T14:27:08Z

src/test/java/net/sf/jabref/logic/importer/fileformat/BibtexParserTest.java

-    public void parseRemovesNewlineInFileField() throws IOException {
-        ParserResult result = BibtexParser.parse(new StringReader("@article{canh05,file = {ups \n\tsala}}"),
+    public void parsePreservesNewlineInAbstractField() throws IOException {
+        ParserResult result = BibtexParser.parse(new StringReader("@article{canh05,abstract = {ups \nsala}}"),
                importFormatPreferences);


Would it make sense to create a BibtexParser.parse(string, prefs) method?

I dunno, amybe @lenhard knows.

Well, it is what the BibtexParser.parse method does internally. If you parse multiple times subsequently, there might be a little performance impact, if you parse only once then it does not matter.

simonharrer · 2016-12-13T14:27:27Z

src/test/java/net/sf/jabref/logic/importer/fileformat/BibtexParserTest.java

                importFormatPreferences);

        Collection<BibEntry> c = result.getDatabase().getEntries();
        BibEntry e = c.iterator().next();
-        assertEquals(Optional.of("ups  sala"), e.getField("file"));
+        assertEquals(Optional.of("ups " + OS.NEWLINE + "sala"), e.getField("abstract"));


Do we have a constant for the abstract field somewhere?

simonharrer · 2016-12-13T14:28:51Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

+        // PDF should be annotated:
+        List<BibEntry> l = XMPUtil.readXMP(pdfFile, xmpPreferences);
+        Assert.assertEquals(1, l.size());
+        assertEqualsBibtexEntry(t1BibtexEntry(), l.get(0));


I do not like to assert the size of a list, because the result would simply say that the count mismatches, not which elements are in the list. Why not use the assertEqualsBibtexEntryList method if there is some which compares a list containing t1BibtexEntry with l?

As far as I know there is no such method or maybe @tobiasdiez knows.

Assert.assertEquals(Collections.singleton(tlBibtexEntry()), l) should work (and please rename l to something better :-)

Uhh... as this relies on the equals implemenation of BibEntry I'm not sure whether this really works. 😉

Yeah, we wanted to use a canonical string representation for the bib entries.

The current implementation of BibEntryAssert also relies on equals so it is very much OK to use the code @tobiasdiez proposed.

Does not work right now author is not normailzed or so:
Expected

[@article{canh05, author = {Crowston, K. and Annabi, H. and Howison, J. and Masango, C.}, booktitle = {Hawaii International Conference On System Sciences (HICSS)}, owner = {oezbek}, timestamp = {2006.05.29}, title = {Effective work practices for floss development: A model and propositions}, url = {http://james.howison.name/publications.html}, year = {2005} }]

Actual

[@article{canh05, author = {K. Crowston and H. Annabi and J. Howison and C. Masango}, booktitle = {Hawaii International Conference On System Sciences (HICSS)}, owner = {oezbek}, timestamp = {2006.05.29}, title = {Effective work practices for floss development: A model and propositions}, url = {http://james.howison.name/publications.html}, year = {2005} }]

I would skip this for now.

simonharrer · 2016-12-13T14:29:06Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

+        // PDF should be annotated:
+        l = XMPUtil.readXMP(pdfFile, xmpPreferences);
+        Assert.assertEquals(1, l.size());
+        assertEqualsBibtexEntry(t2BibtexEntry(), l.get(0));


koppor

In general: LGTM

As usual, I vote for keeping and adapting tests. 😇 Find the comments inside. ☁️

koppor · 2016-12-15T12:05:17Z

src/main/java/net/sf/jabref/cli/XMPUtilMain.java

@@ -176,7 +176,7 @@ private static void usage() {
        System.out.println("  xmpUtil <bib> <pdf>");
        System.out.println("");
        System.out
-        .println("To report bugs visit http://jabref.sourceforge.net");
+        .println("To report bugs visit https://github.com/JabRef/jabref/issues");


https://issues.jabref.org, pleae

koppor · 2016-12-15T12:06:21Z

src/main/java/net/sf/jabref/logic/openoffice/OOBibStyle.java

        this.prefs = Objects.requireNonNull(prefs);
        this.encoding = StandardCharsets.UTF_8;
        setDefaultProperties();
-        initialize(OOBibStyle.class.getResource(resourcePath).openStream());
+        initialize(OOBibStyle.class.getResourceAsStream(resourcePath));


Maybe we can wrap it inside Objects.requireNonNull() and also add some Objects.requireNonNull for resourcePath and prefs before lin 153

koppor · 2016-12-15T12:07:42Z

src/test/java/net/sf/jabref/logic/importer/fileformat/BibtexParserTest.java

@@ -786,19 +786,6 @@ public void parseWarnsAboutUnmatchedContentInEntry() throws IOException {
    }

    @Test
-    @Ignore("Ignoring because this is an edge case")


I would like to know, what's happening here. Can the test be adapted to the current behavior and possibly commented, what should happen?

I would need that if someone comes over and wants to replace our parser 😇

It just doesn't work right now as @lenhard says. So there is no sense to test it at the moment.

JabRef returns one entry, which pretty good. I would like to have this checked in tests if we adress it.

The test has to be adapted accordingly.

Result is

@article{test, author = {author bracket #too##much#} }

Would it be OK if you include this as expected case, even if the } gets lost...

This documents the drawback of JabRef.

OK, additional documentation at https://help.jabref.org/en/Bibtex would be better, but none of us has time for that.

koppor · 2016-12-15T12:09:39Z

src/test/java/net/sf/jabref/logic/layout/LayoutTest.java

@@ -86,15 +82,6 @@ public void testHTMLChar() throws IOException {
    }

    @Test
-    @Ignore


Why not adapting it to the current behavior? I think, we drop <br> now, which is fine. Why not leaving the additional test?

Please do not delete the test. Just remove @Ignore and remove <br>.

If that's really what we want to achieve here \n\n -> ' '
I'm not sure about this that's why I removed the test.

In other words: That is the current behavior, yes. But should it really be the correct behavior? In my opinion we should only test correct behavior not as is behavior. As the correct behavior for example
is not working right now I cannot really test it.

I think, we decided to remove newlines in non-multiline fields. Is it #357?

Even if that is not the desired behavior, I would add // TODO: The correct behavior is to indicate that someone should work on it.

Alternative: open an issue on GitHub. - I am currently optimizing (i) to keep the number of opened issues low and (ii) to have documentation close to the code and not spread around.

koppor · 2016-12-15T12:13:19Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

+        // PDF should be annotated:
+        List<BibEntry> l = XMPUtil.readXMP(pdfFile, xmpPreferences);
+        Assert.assertEquals(1, l.size());
+        assertEqualsBibtexEntry(t1BibtexEntry(), l.get(0));


Yeah, we wanted to use a canonical string representation for the bib entries.

The current implementation of BibEntryAssert also relies on equals so it is very much OK to use the code @tobiasdiez proposed.

koppor · 2016-12-15T12:16:14Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

+        if (!tempBib.delete()) {
+            // TODO: file is still locked
+            System.err.println("Cannot delete temporary file");
+        }


Why not just calling deleteOnExit()? This would prevent these kind of issues.

koppor · 2016-12-15T12:19:07Z

src/test/java/net/sf/jabref/logic/xmp/XMPUtilTest.java

+            try {
+                XMPUtilMain.main(new String[]{"OezbekC06", tempBib.getAbsolutePath(), pdfFile.getAbsolutePath()});
+            } finally {
+                System.setOut(sysOut);
            }
        }


OK for leave as is - I copied @simonharrer's comment to JabRef#6.

# Conflicts: # src/test/java/net/sf/jabref/logic/importer/fileformat/BibtexParserTest.java

Remove test

2111468

stefan-kolb added 2 commits December 12, 2016 17:05

Merge branch 'master' into enable-tests

35e7349

Remove another test

f83a21f

Fix OpenOffice test

bd80523

stefan-kolb force-pushed the enable-tests branch from 1ab9cbb to bd80523 Compare December 12, 2016 16:48

Formatting

37ab9ec

Reenable author junior test

0407841

Remove unimplemented edge case

8278b82

Invert parsing tests for abstract field

57226fe

stefan-kolb force-pushed the enable-tests branch from 78ee026 to 57226fe Compare December 13, 2016 09:43

stefan-kolb added 2 commits December 13, 2016 10:49

Imports again

f144141

Fix XMP test

2563fe2

stefan-kolb force-pushed the enable-tests branch from 6ba6b62 to 2563fe2 Compare December 13, 2016 10:07

stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 13, 2016

Double line breaks cannot be replaced anymore as the Bibtex parser st…

05af767

…rips duplicated whitespaces

stefan-kolb force-pushed the enable-tests branch from 8731bbb to 05af767 Compare December 13, 2016 10:22

Imports...

a624fb2

stefan-kolb requested review from koppor and lenhard December 13, 2016 13:45

lenhard suggested changes Dec 13, 2016

View reviewed changes

Reformat test a little bit

c316eda

simonharrer reviewed Dec 13, 2016

View reviewed changes

Field constants

4449e64

lenhard approved these changes Dec 13, 2016

View reviewed changes

koppor requested changes Dec 15, 2016

View reviewed changes

stefan-kolb added 5 commits December 21, 2016 11:00

Null checks

0680fd4

Use temp folder

4e6840b

Different URL

2865301

Readd tests

26f2fb0

Merge branch 'master' into enable-tests

ad6103f

# Conflicts: # src/test/java/net/sf/jabref/logic/importer/fileformat/BibtexParserTest.java

stefan-kolb merged commit 5a5ff12 into master Dec 22, 2016

stefan-kolb deleted the enable-tests branch December 22, 2016 13:55

@@ @@ -86,15 +82,6 @@ public void testHTMLChar() throws IOException { @@
                   }
                   @Test
-                  @Ignore

Reenable more tests #2371

Reenable more tests #2371

Conversation

stefan-kolb commented Dec 12, 2016 • edited Loading

stefan-kolb commented Dec 12, 2016

stefan-kolb commented Dec 12, 2016

stefan-kolb commented Dec 12, 2016

tobiasdiez commented Dec 12, 2016

koppor commented Dec 12, 2016

lenhard commented Dec 13, 2016

stefan-kolb commented Dec 13, 2016

matthiasgeiger commented Dec 13, 2016

lenhard commented Dec 13, 2016

stefan-kolb commented Dec 13, 2016 • edited Loading

lenhard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefan-kolb Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

stefan-kolb commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefan-kolb commented Dec 12, 2016 •

edited

Loading

stefan-kolb commented Dec 13, 2016 •

edited

Loading

stefan-kolb Dec 13, 2016 •

edited

Loading