Added initial tests for ExportFormat #1010

oscargus · 2016-03-21T14:25:04Z

Really trivial and early test, but could maybe avoid bugs like #1001 to happen (and form a foundation to extend on).

lenhard · 2016-03-21T16:41:34Z

src/test/java/net/sf/jabref/exporter/ExportFormatTest.java

+            MetaData metaData = new MetaData();
+            exportFormat.performExport(db, metaData, filename, charset, entries);
+        } catch (IOException e) {
+            Assert.fail("Exception caught: " + e.toString() + e.getMessage());


It would probably be more readable code if you just skip the try-catch here. The tests will fail anyway if the method is terminated by an exception and we will get all the exception info we need. There is no need to explicitly build an exception string as you do here.

Some code quality tools might unnecessarily complain about missing assertions, though...

Makes sense. I guess one can figure out some sort of assertion, such as the generated file actually existing...

@lenhard That should be a general rule in my opinion, not to catch Exceptions ins Tests.
Edit// At the moment there are tests that fail but omit stacktrace. I'lll create a new issue for this

tobiasdiez · 2016-03-21T17:09:35Z

Mhh....I am missing an assertion in these tests, i.e. reading the exported file and checking that it is empty.

oscargus · 2016-03-22T08:52:11Z

I have changed the code based on all(?) comments. Major implications:

Now, all exporters returns an empty file for an empty set of entries and are tested for this. Earlier, some heading information was generated
All exporters are now tested that they throw an NPE with a null database or null entries list
The change that fixes error when exporting to endote and ris format #1001 is in practice reverted and solved such that the ExportFormat calls add all entries instead of null for all entries
The test disabled in Disabled NPE test #1005 is active again
I had to break the new guideline in Don't catch exceptions in Unit test #1012 to be able to test all exporters for NPEs in a simple way, if there's a better way please let me know

(This shows that adding a quite crappy initial test can lead to good things. :-))

oscargus · 2016-03-22T09:07:01Z

As the export is now not performed for empty databases (entries lists), as a side effect, the coverage is more accurate now as well. Earlier it increased quite a bit, but no actual testing was done (well, apart from that the code executed without any errors).

…ignored BibDatabaseWriter change

tobiasdiez · 2016-03-22T10:33:34Z

src/test/java/net/sf/jabref/exporter/ExportFormatTest.java

+    @Test
+    public void testExportingEmptyDatabaseYieldsEmptyFile() throws Exception {
+        BibDatabase database = new BibDatabase();
+        for (Map.Entry<String, IExportFormat> exportFormats : ExportFormats.getExportFormats().entrySet()) {


You can use parametrized tests instead of the for loop. Then also the the string argument to assertEquals is no longer needed.

Any good pointer/example? (It seemed like it should be possible somehow...)

https://github.com/junit-team/junit4/wiki/Parameterized-tests and the tests in FormatterTests

tobiasdiez · 2016-03-22T10:38:57Z

This looks way better then the initial PR. Good job!

oscargus · 2016-03-22T13:03:41Z

Not obvious that Parameterized is executed before BeforeClass, but once I figured that out...

tobiasdiez · 2016-03-22T13:34:20Z

src/test/java/net/sf/jabref/exporter/ExportFormatTest.java

-    public void setUp() {
-        Globals.prefs = JabRefPreferences.getInstance();
-        ExportFormats.initAllExports();
+    @BeforeClass


I think before is preferable to beforeclass since then every test gets a fresh database and the potentially modifications of the previous test do not influence the next test.

OK! I do not fully understand the difference and usually use Before. Now I just mimicked FormatterTest to see if it helped (but the problem was not related to that).

http://stackoverflow.com/questions/20295578/difference-between-before-and-beforeclass

And regarding providing an actual database, the current method is not the best approach (with statics). However, it seems like the tests needs to be rather specific anyway for the different export formats so maybe they can't be parameterized in a good way anyhow.

Yes, I second this. Every export format should have its own test class, which tests the actually export.

Btw, is there a way to add a test which is not included in the coverage check? It would make sense to try to export a non-empty database for every export format just to make sure that no exceptions are thrown, but probably one wouldn't like to include these test in the coverage report...

tobiasdiez · 2016-03-22T13:36:36Z

LGTM

oscargus · 2016-03-23T17:42:38Z

I think this is probably as far as we get right now and that the code is quite OK since it is rather general and only tests what is actually tested (or how to formulate it...).

Added initial tests for ExportFormat

7e0bd7c

oscargus added testing status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers labels Mar 21, 2016

lenhard reviewed Mar 21, 2016
View reviewed changes

Improved tests and always include all entries instead of null

50152cc

oscargus force-pushed the exportformattest branch from 43089f5 to 4a6f0a4 Compare March 22, 2016 08:53

Improved the tests to test all exporters and restored the previously …

d316463

…ignored BibDatabaseWriter change

oscargus force-pushed the exportformattest branch from 4a6f0a4 to d316463 Compare March 22, 2016 09:10

tobiasdiez reviewed Mar 22, 2016
View reviewed changes

Parameterized tests

b9bf04a

oscargus force-pushed the exportformattest branch from 61962f8 to b9bf04a Compare March 22, 2016 13:12

tobiasdiez reviewed Mar 22, 2016
View reviewed changes

oscargus merged commit e477631 into JabRef:master Mar 23, 2016

oscargus deleted the exportformattest branch March 23, 2016 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added initial tests for ExportFormat #1010

Added initial tests for ExportFormat #1010

oscargus commented Mar 21, 2016

lenhard Mar 21, 2016

oscargus Mar 21, 2016 via email

Siedlerchr Mar 21, 2016

tobiasdiez commented Mar 21, 2016

oscargus commented Mar 22, 2016

oscargus commented Mar 22, 2016

tobiasdiez Mar 22, 2016

oscargus Mar 22, 2016

tobiasdiez Mar 22, 2016

tobiasdiez commented Mar 22, 2016

oscargus commented Mar 22, 2016

tobiasdiez Mar 22, 2016

oscargus Mar 22, 2016

tobiasdiez Mar 22, 2016

oscargus Mar 22, 2016

tobiasdiez Mar 22, 2016

oscargus Mar 22, 2016

tobiasdiez commented Mar 22, 2016

oscargus commented Mar 23, 2016

Added initial tests for ExportFormat #1010

Added initial tests for ExportFormat #1010

Conversation

oscargus commented Mar 21, 2016

Choose a reason for hiding this comment

oscargus Mar 21, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Mar 21, 2016

oscargus commented Mar 22, 2016

oscargus commented Mar 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasdiez commented Mar 22, 2016

oscargus commented Mar 22, 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

tobiasdiez commented Mar 22, 2016

oscargus commented Mar 23, 2016