Add test case for other and unknown type #2152

tschechlovdev · 2016-10-10T12:59:30Z

Regarding: #29
I've added a test case for reading and writing a file with other and unknwon type.

oscargus · 2016-10-11T18:31:47Z

src/test/java/net/sf/jabref/logic/bibtex/BibEntryWriterTest.java

+                "}";
+
+        // read in bibtex string
+        ParserResult result = BibtexParser.parse(new StringReader(bibtexEntry), importFormatPreferences);


Please use new BibtexParser(importFormatPreferences).parse(new StringReader(bibtexEntry)) (or Collection<BibEntry> entries = new BibtexParser(importFormatPreferences).parseEntries(bibtexEntry);.

oscargus · 2016-10-11T18:32:03Z

src/test/java/net/sf/jabref/logic/bibtex/BibEntryWriterTest.java

+
+        // read in bibtex string
+        ParserResult result = BibtexParser.parse(new StringReader(bibtexEntry), importFormatPreferences);
+        Collection<BibEntry> entries = result.getDatabase().getEntries();


tobiasdiez · 2016-10-12T11:11:36Z

src/test/java/net/sf/jabref/logic/bibtex/BibEntryWriterTest.java

+        Collection<BibEntry> entries = result.getDatabase().getEntries();
+        BibEntry entry = entries.iterator().next();
+
+        String resourceName = "/testbib/othertype.bib";


Please remove the assert statement here. Only one assert / failure reason per test. If you think that is valuable to test that the other type is written correctly, then extract this test to a new test method (I would actually prefer two tests for writing and reading separately instead of a roundtrip test)

Done. I've moved the assert to a new test method.

tobiasdiez · 2016-10-12T11:12:21Z

src/test/java/net/sf/jabref/logic/bibtex/BibEntryWriterTest.java

+        Collection<BibEntry> entries = result.getDatabase().getEntries();
+        BibEntry entry = entries.iterator().next();
+
+        String resourceName = "/testbib/reallyunknowntype.bib";


same remark as above.

tobiasdiez · 2016-10-12T13:05:25Z

src/test/java/net/sf/jabref/logic/bibtex/BibEntryWriterTest.java

+
+    @Test
+    public void readReallyUnknownTypeTest() throws Exception {
+        String bibtexEntry = "@ReallyUnknownType{test," + OS.NEWLINE +


If you just write it, then there is no need to read it from a string...thus please create the BibEntry by hand in these cases.

The purpose of this method was to ensure that the String was read in correctly. The other test method is still a roundtrip test. Then I don't think it makes much sense to just create the entry here by hand.

So I think I should change the roundtrip test to a test to just write the entry (and there I will create the entry by hand).

If you have a test for reading something, then you should place it in the ParserTest ;-)

Ok, removed it from here and placed it to the ParserTest 😄

Siedlerchr

Looks good!

Siedlerchr · 2016-10-12T15:36:42Z

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

+        Collection<BibEntry> entries = new BibtexParser(importFormatPreferences).parseEntries(bibtexEntry);
+        BibEntry entry = entries.iterator().next();
+
+        String resourceName = "/testbib/reallyunknowntype.bib";


Why is the resourceName necessary? The other tests don't use it

I've wanted to test explicit against the testfile "reallyunknowntype.bib". If I see right, the other tests don't use such test files.

I think this is not necessary in this case. If you really want to test it, then please use the Resources directory handling, like in theBibTexImportetTest.class and do not hardcode the relative dir.
Paths.get(BibtexImporterTest.class.getResource("BibtexImporter.examples.bib").toURI());

Yes better compare it directly against a hand-created bibEntry (copy from above).
Then you can use assertEquals(singletonlist of expected entry, entries).

Apart from this last remark (sorry for being pedantic) this looks good to me and can be merged.

Ok, I've created the bibentry now by hand.

tobiasdiez · 2016-10-17T14:23:52Z

Thanks for the follow-up!

add test case for other and unknown type

e4a21db

tschechlovdev added stupro labels Oct 10, 2016

boceckts added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed stupro-ready-for-internal-review labels Oct 11, 2016

boceckts changed the title ~~[WIP] Add test case for other and unknown type~~ Add test case for other and unknown type Oct 11, 2016

oscargus reviewed Oct 11, 2016

View reviewed changes

tobiasdiez requested changes Oct 12, 2016

View reviewed changes

tobiasdiez removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 12, 2016

tschechlovdev added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Oct 12, 2016

tobiasdiez reviewed Oct 12, 2016

View reviewed changes

address comments

6b93549

tschechlovdev force-pushed the issue29 branch from 2803c8c to 6b93549 Compare October 12, 2016 14:46

move tests for reading to the parser test

e0d8dd4

Siedlerchr approved these changes Oct 12, 2016

View reviewed changes

Siedlerchr reviewed Oct 12, 2016

View reviewed changes

tschechlovdev force-pushed the issue29 branch from ef44cf9 to d4a73d4 Compare October 12, 2016 22:15

create bib entry by hand

6b79a29

tschechlovdev force-pushed the issue29 branch from d4a73d4 to 6b79a29 Compare October 12, 2016 22:15

tobiasdiez merged commit 8b65bf3 into JabRef:master Oct 17, 2016

tschechlovdev deleted the issue29 branch October 19, 2016 06:57

tschechlovdev mentioned this pull request Oct 19, 2016

Include test cases for other types JabRef/jabref-koppor#29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test case for other and unknown type #2152

Add test case for other and unknown type #2152

tschechlovdev commented Oct 10, 2016

oscargus Oct 11, 2016

tschechlovdev Oct 12, 2016

oscargus Oct 11, 2016

tobiasdiez Oct 12, 2016

tschechlovdev Oct 12, 2016

tobiasdiez Oct 12, 2016

tobiasdiez Oct 12, 2016

tschechlovdev Oct 12, 2016

tobiasdiez Oct 12, 2016

tschechlovdev Oct 12, 2016

Siedlerchr left a comment

Siedlerchr Oct 12, 2016

tschechlovdev Oct 12, 2016

Siedlerchr Oct 12, 2016

tobiasdiez Oct 12, 2016

tschechlovdev Oct 12, 2016

tobiasdiez commented Oct 17, 2016

Add test case for other and unknown type #2152

Add test case for other and unknown type #2152

Conversation

tschechlovdev commented Oct 10, 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

Siedlerchr 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

tobiasdiez commented Oct 17, 2016