Migrate importer tests to JUnit5 #3665

lenhard · 2018-01-25T13:00:14Z

In the course of #3642 (comment) we realized that our current (JUnit4) parameterized tests are not executed on the (JUnit5) travis build. This is tracked here: #3645 I think this is a huge problem, because we are missing out on a lot of validation here.

This PR is a migration of the paramterized RIS tests from JUnit 4 to JUnit 5. I'd like to put this forward to discussion, so that we can agree on how our parameterized JUnit 5 tests are to look like. When we have agreed, we can migrate all tests. So, please look at the code and write if you are happy with this.

Note that parameters cannot be resolved for lifecycle methods (@BeforeEach) in JUnit 5, hence the structure in the PR.

Change in CHANGELOG.md described
Tests created for changes
Screenshots added (for bigger UI changes)
Manually tested changed features in running JabRef
Check documentation status (Issue created for outdated help page at help.jabref.org?)

tobiasdiez

In general, this looks good to me and seems to be according to the intended use of parametrized tests according to the documentation of JUnit5 (albeit, I really dislike the string-based argument to "MethodSource", but well, this is how it is).

I have two suggestions, but they are not really thought-trough so there might be good reasons to not pursue them further:

You can have parameterized tests with more than one argument (JUnit5 doc, under "MethodSource"). Thus, it seems natural to provide the "ris" path and the "bib" path both as arguments (preferable both really as nio.Paths instead of strings). This could simplify the setup-construction.
If I remember correctly, we have such a files-based parametrized test class for every importer, with exactly the same code. It might make sense to combine all of them and pass the importer as an additional parameter, i.e. testImportEntries(Importer importerToUse, Path fileToImport, Path referenceBibFile).

lenhard · 2018-01-25T15:49:20Z

@tobiasdiez Thanks a lot for the feedback!

Regarding test arguments: I don't really see the point in splitting the single argument into two. We have used the convention that the .bib and .importerFormat files have the same name and why not keep leveraging that? Also, I find it better to do the path conversion in one place only, but if we want to pass a stream of paths we always have to do that beforehand
Regarding unification of importer tests: I have refactored around a little and arrived at a solution that should work for all importers. I quite like how that condenses the RISImporterTestFiles class.

Fun fact: I may have used JUnit 5 paramterization, but so far the assertions were still JUnit 4. I'll change that in the following.

tobiasdiez

That's a nice refactoring! Like it.

Siedlerchr · 2018-01-26T09:13:34Z

src/test/java/org/jabref/logic/bibtex/BibEntryAssert.java

-        Assert.assertNotNull(clazz);
-        Assert.assertNotNull(resourceName);
-        Assert.assertNotNull(entry);
+        Assertions.assertNotNull(clazz);


I would suggest, using a static import as before makes it more readable

Siedlerchr · 2018-01-26T09:15:49Z

src/test/java/org/jabref/logic/importer/fileformat/RISImporterTestFiles.java

-    public static Collection<String> fileNames() {
-        return Arrays.asList("RisImporterTest1", "RisImporterTest3", "RisImporterTest4a", "RisImporterTest4b",
+    private static Stream<String> fileNames() {
+        return Stream.of("RisImporterTest1", "RisImporterTest3", "RisImporterTest4a", "RisImporterTest4b",


It would be nice to have the RIS Importer collect the files dynamically like the other file tests do, e.g. starts with RIS and contains Test

I will have a look at that and I will also refactor the remainder of the importer tests into this structure.

I have extracted the relevant logic now for the RIS tests. However, it turns out that the file reading logic in all importer tests is broken (except for the RIS tests now). It just does not work, no files are found and in that case JUnit 4 didn't complain. JUnit 5 complains that there are no parameters.

I am not sure if the importer tests never worked and people where just happy that they were green, or if they worked and got broken at some point in time. I'll gradually try to get the tests working now.

Siedlerchr · 2018-01-26T15:49:35Z

The importer tests where running for each file, but that seems to have been ignored in the migration process by the junit 5 comp.
I looked at this, too but didn't find a way to get it to work again.

lenhard · 2018-01-26T15:51:33Z

Yes, indeed. I'll gradually go through all parameterized importer tests and try to get them working in JUnit 5 syntax again as part of this PR.

lenhard · 2018-01-26T16:37:52Z

Turns out, quite a lot of tests for different importers (BibTexML, Medline tested so far) actually fail and this is down to the implementations of the importers. Seems like these tests have never been executed before...

I am not sure what to do about this here. The point of this PR was a refactoring of the testing code, not a repair of broken importers.

Siedlerchr · 2018-01-26T16:41:32Z

I would merge this if all tests are at least executed. Or better you merge this in a new branch where we will fix the importer then

Siedlerchr · 2018-01-27T12:04:55Z

src/test/java/org/jabref/logic/importer/fileformat/ImporterTestEngine.java

+
+public class ImporterTestEngine {
+
+    private static final String TEST_RESOURCES = "src/test/resources/org/jabref/logic/importer/fileformat";


Maybe better to use Paths.get(ImporterTestEngine.class).getResource("").toUri())

I see you have the getPath already

Siedlerchr · 2018-01-27T12:33:34Z

I converted and fixed some fetcher tests in another PR. I found that the GVK Test would also be a goo candidate for using the same schema as here

Siedlerchr · 2018-01-30T15:56:51Z

LGTM!, if you merge master in your branch you will get the fetcher test fixed as well

lenhard · 2018-01-31T14:01:37Z

Ok, this can be merged from my point of view.

Although this is already a huge refactoring of the existing tests, there's certainly more that can be improved. I have a few things in mind, but I would rather keep them out of this already big PR. I'll tackle them later when this is merged.

tobiasdiez

I have to admit, that I didn't checked very statement and just scrolled through the code.
The only thing I don't understand is why the code coverage stays almost the same although now some tests should run which didn't run before. However, https://codecov.io/gh/JabRef/jabref/tree/migrate-parameter-tests/src/main/java/org/jabref/logic/importer/fileformat still shows almost 0% coverage for some importers. Any idea why this is the case?

lenhard · 2018-01-31T16:46:00Z

That looks weird indeed. Looking at the first one: https://codecov.io/gh/JabRef/jabref/src/migrate-parameter-tests/src/main/java/org/jabref/logic/importer/fileformat/BibTeXMLImporter.java the importDatabase method is recorded as not covered, but when I execute the tests in Intellij there are twenty test files for that method alone.

The key seems to be that the missing methods are the ones that use JUnit5 parameterized tests. It seems that Codecov does not record the coverage of these tests, but they are the core tests of the whole package.

But maybe the problem is still travis? How do I see from our travis output that the tests are really executed? I can push a failing test deliberately, but there surely is a better way?

tobiasdiez · 2018-01-31T16:59:29Z

Since the travis log contains statement like

13:43:57.686 [main] WARN org.jabref.logic.msbib.MSBibDatabase - Could not parse document

I suppose the tests are correctly invoked (in fact, the travis log exploded even more). Probably there is a problem with the gradle task jacocoJunit5TestReport (but there is no need to fix this in this PR...)

Siedlerchr · 2018-01-31T17:06:59Z

I would merge this in and I think tthere might be a solution for the reports:
junit-team/junit5#1024

* upstream/master: (155 commits) Update DEVELOPERS Update README.md [WIP] File link deletion dialog improvements (#3690) Update guava (#3692) Fix some FX Thread issue (#3691) Fixed typo, mayor to major (#3685) add Office xml test for latex free fields (#3680) Migrate importer tests to JUnit5 (#3665) fix typo Update slf4j-api from 1.8.0-beta0 -> 1.8.0-beta1 Update richtext from 0.8.1 -> 0.8.2 Update junit from 5.1.0-M1 -> 5.1.0-M2 Update checkstyle from 8.7 -> 8.8 Export all fields with their latex free equivalent (#3675) try around with exckluding tag (#3676) Fixes #3648: Chained modifiers work again (#3670) Update gradle from 4.4.1 to 4.5 and tweak .gitattributes Fix 2633 saving creates new database (#3674) New translations JabRef_en.properties (French) Implements #1664: group based on aux file (#3444) ... # Conflicts: # CHANGELOG.md # CONTRIBUTING.md # build.gradle # gradle/wrapper/gradle-wrapper.jar # gradle/wrapper/gradle-wrapper.properties # scripts/syncLang.py # src/main/java/org/jabref/FallbackExceptionHandler.java # src/main/java/org/jabref/Globals.java # src/main/java/org/jabref/JabRefMain.java # src/main/java/org/jabref/cli/ArgumentProcessor.java # src/main/java/org/jabref/cli/JabRefCLI.java # src/main/java/org/jabref/collab/EntryChange.java # src/main/java/org/jabref/collab/EntryDeleteChange.java # src/main/java/org/jabref/collab/FileUpdateListener.java # src/main/java/org/jabref/collab/StringAddChange.java # src/main/java/org/jabref/collab/StringChange.java # src/main/java/org/jabref/collab/StringNameChange.java # src/main/java/org/jabref/collab/StringRemoveChange.java # src/main/java/org/jabref/gui/BasePanel.java # src/main/java/org/jabref/gui/ClipBoardManager.java # src/main/java/org/jabref/gui/DefaultInjector.java # src/main/java/org/jabref/gui/DialogService.java # src/main/java/org/jabref/gui/EntryTypeDialog.java # src/main/java/org/jabref/gui/JabRefFrame.java # src/main/java/org/jabref/gui/actions/IntegrityCheckAction.java # src/main/java/org/jabref/gui/collab/ChangeScanner.java # src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java # src/main/java/org/jabref/gui/copyfiles/CopyFilesTask.java # src/main/java/org/jabref/gui/customjfx/CustomJFXPanel.java # src/main/java/org/jabref/gui/desktop/JabRefDesktop.java # src/main/java/org/jabref/gui/entryeditor/DeprecatedFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/EntryEditor.css # src/main/java/org/jabref/gui/entryeditor/EntryEditor.java # src/main/java/org/jabref/gui/entryeditor/FieldsEditorTab.java # src/main/java/org/jabref/gui/entryeditor/OptionalFields2Tab.java # src/main/java/org/jabref/gui/entryeditor/OptionalFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/OtherFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/RequiredFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/SourceTab.java # src/main/java/org/jabref/gui/entryeditor/UserDefinedFieldsTab.java # src/main/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationTabController.java # src/main/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationTabViewModel.java # src/main/java/org/jabref/gui/exporter/ExportAction.java # src/main/java/org/jabref/gui/exporter/ExportFileFilter.java # src/main/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtil.java # src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java # src/main/java/org/jabref/gui/fieldeditors/EditorValidator.java # src/main/java/org/jabref/gui/fieldeditors/FieldEditors.java # src/main/java/org/jabref/gui/fieldeditors/FileListEditorTransferHandler.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java # src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java # src/main/java/org/jabref/gui/fieldeditors/MapBasedEditorViewModel.java # src/main/java/org/jabref/gui/groups/GroupDialog.java # src/main/java/org/jabref/gui/groups/GroupSidePane.java # src/main/java/org/jabref/gui/groups/GroupTreeController.java # src/main/java/org/jabref/gui/help/AboutDialogViewModel.java # src/main/java/org/jabref/gui/help/HelpAction.java # src/main/java/org/jabref/gui/importer/ImportFileFilter.java # src/main/java/org/jabref/gui/importer/ImportFormats.java # src/main/java/org/jabref/gui/maintable/MainTableSelectionListener.java # src/main/java/org/jabref/gui/preftabs/AppearancePrefsTab.java # src/main/java/org/jabref/gui/preftabs/EntryEditorPrefsTab.java # src/main/java/org/jabref/gui/preftabs/PreviewPrefsTab.java # src/main/java/org/jabref/gui/search/GlobalSearchBar.java # src/main/java/org/jabref/gui/util/BindingsHelper.java # src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java # src/main/java/org/jabref/gui/util/DefaultTaskExecutor.java # src/main/java/org/jabref/gui/util/FileUpdateListener.java # src/main/java/org/jabref/gui/worker/CitationStyleToClipboardWorker.java # src/main/java/org/jabref/logic/bibtexkeypattern/BibtexKeyGenerator.java # src/main/java/org/jabref/logic/bibtexkeypattern/BracketedPattern.java # src/main/java/org/jabref/logic/citationstyle/CitationStyle.java # src/main/java/org/jabref/logic/citationstyle/CitationStyleCache.java # src/main/java/org/jabref/logic/citationstyle/CitationStyleGenerator.java # src/main/java/org/jabref/logic/exporter/BibTeXMLExporter.java # src/main/java/org/jabref/logic/exporter/ExportFormats.java # src/main/java/org/jabref/logic/exporter/IExportFormat.java # src/main/java/org/jabref/logic/exporter/MSBibExporter.java # src/main/java/org/jabref/logic/exporter/ModsExporter.java # src/main/java/org/jabref/logic/exporter/OpenDocumentSpreadsheetCreator.java # src/main/java/org/jabref/logic/exporter/OpenOfficeDocumentCreator.java # src/main/java/org/jabref/logic/exporter/TemplateExporter.java # src/main/java/org/jabref/logic/importer/ImportFormatReader.java # src/main/java/org/jabref/logic/importer/WebFetchers.java # src/main/java/org/jabref/logic/importer/fetcher/IacrEprintFetcher.java # src/main/java/org/jabref/logic/integrity/FieldCheckers.java # src/main/java/org/jabref/logic/integrity/IntegrityCheck.java # src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java # src/main/java/org/jabref/logic/shared/DBMSProcessor.java # src/main/java/org/jabref/logic/shared/DBMSSynchronizer.java # src/main/java/org/jabref/logic/util/FileType.java # src/main/java/org/jabref/logic/util/JavaVersion.java # src/main/java/org/jabref/logic/util/io/CiteKeyBasedFileFinder.java # src/main/java/org/jabref/logic/util/io/FileFinder.java # src/main/java/org/jabref/logic/util/io/FileUtil.java # src/main/java/org/jabref/logic/util/io/RegExpBasedFileFinder.java # src/main/java/org/jabref/model/auxparser/AuxParserResult.java # src/main/java/org/jabref/model/database/BibDatabaseContext.java # src/main/java/org/jabref/model/entry/FieldProperty.java # src/main/java/org/jabref/model/pdf/FileAnnotation.java # src/main/java/org/jabref/model/util/FileUpdateListener.java # src/main/java/org/jabref/preferences/CustomExportList.java # src/main/java/org/jabref/preferences/JabRefPreferences.java # src/test/java/org/jabref/TestArchitectureTests.java # src/test/java/org/jabref/gui/externalfiles/AutoSetFileLinksUtilTest.java # src/test/java/org/jabref/gui/importer/EntryFromFileCreatorManagerTest.java # src/test/java/org/jabref/logic/bibtexkeypattern/BibtexKeyGeneratorTest.java # src/test/java/org/jabref/logic/bibtexkeypattern/MakeLabelWithDatabaseTest.java # src/test/java/org/jabref/logic/exporter/MsBibExportFormatTest.java # src/test/java/org/jabref/logic/importer/ImportFormatReaderTestParameterless.java # src/test/java/org/jabref/logic/importer/fetcher/ArXivTest.java # src/test/java/org/jabref/logic/importer/fetcher/AstrophysicsDataSystemTest.java # src/test/java/org/jabref/logic/importer/fetcher/CrossRefTest.java # src/test/java/org/jabref/logic/importer/fetcher/DBLPFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/DiVATest.java # src/test/java/org/jabref/logic/importer/fetcher/DoiFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/FulltextFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/GvkFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/IacrEprintFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/IsbnFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/IsbnViaChimboriFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/IsbnViaEbookDeFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/LibraryOfCongressTest.java # src/test/java/org/jabref/logic/importer/fetcher/MedlineFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/MrDLibFetcherTest.java # src/test/java/org/jabref/logic/importer/fetcher/SpringerLinkTest.java # src/test/java/org/jabref/logic/importer/fetcher/TitleFetcherTest.java # src/test/java/org/jabref/logic/importer/fileformat/BibtexParserTest.java # src/test/java/org/jabref/logic/importer/fileformat/FreeCiteImporterTest.java # src/test/java/org/jabref/logic/l10n/LocalizationConsistencyTest.java # src/test/java/org/jabref/logic/shared/DBMSConnectionTest.java # src/test/java/org/jabref/logic/shared/DBMSProcessorTest.java # src/test/java/org/jabref/logic/shared/DBMSSynchronizerTest.java # src/test/java/org/jabref/logic/shared/DBMSTypeTest.java # src/test/java/org/jabref/logic/shared/SynchronizationTestEventListener.java # src/test/java/org/jabref/logic/shared/SynchronizationTestSimulator.java # src/test/java/org/jabref/logic/shared/TestConnector.java # src/test/java/org/jabref/logic/util/BracketedPatternTest.java # src/test/java/org/jabref/logic/util/io/CiteKeyBasedFileFinderTest.java # src/test/java/org/jabref/logic/util/io/RegExpBasedFileFinderTests.java # src/test/java/org/jabref/testutils/category/FetcherTest.java

Migrate paramterized RIS tests to JUnit5

d99f273

lenhard added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 25, 2018

tobiasdiez requested changes Jan 25, 2018

View reviewed changes

Extract generic importer testing code to new class

5412c59

lenhard added 2 commits January 25, 2018 16:52

Switch to JUnit5 assertions in BibEntryAssert

f03f2f9

Remove unused imports

2462ec8

tobiasdiez approved these changes Jan 25, 2018

View reviewed changes

Siedlerchr reviewed Jan 26, 2018

View reviewed changes

lenhard added 2 commits January 26, 2018 11:09

Use static import for Assertion when possible

8cd7cb0

Extract file collection from importer test classes

92e23cf

lenhard removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Jan 26, 2018

lenhard added 2 commits January 26, 2018 16:33

Refactor biblioscape importer tests

0fbc8d2

Refactor BiblioscapeImporterTestTypes to JUnit5 syntax

0d448ac

Refactor CopacImporterTestFiles

5935614

Siedlerchr reviewed Jan 27, 2018

View reviewed changes

lenhard added 8 commits January 29, 2018 15:04

Migrate medline importer tests to JUnit5

6e779fe

Convert BibTeXML importer tests to JUnit5

1ab49ec

Move several non-parametric importer test classes to JUnit5

e5cbd4a

Refactor Medline importer tests to JUni5

0e7f5fc

Migrate parameterized MODS importer tests to JUnit5

aebeccf

Migrate MrDLib importer tests to JUnit5

14a710d

Migrate MsBibImporter tests to JUnit5

824920d

Migrate OvidImporter tests to JUnit5

f8e7c47

Siedlerchr mentioned this pull request Jan 30, 2018

Export all fields with their latex free equivalent for MS Office #3675

Merged

5 tasks

lenhard added 17 commits January 31, 2018 11:43

Migrate additional BibTeXML tests to JUnit5

5d01fd7

Merge branch 'master' into migrate-parameter-tests

d4b9d4b

Refactor and restructure BibTexParser tests

9accdca

Refactor CopacImporter tests

97c0017

Refactor BibTeXMLImporter tests

4e1667f

Refactor EndnoteImporter tests

d5b9799

Refactor FreeCiteImporter tests

e014227

Refactor InspecImporter tests

a798b01

Refactor IsiImporter tests

b3205ef

Refactor MedlineImporter tests

0367771

Refactor MrDLibImporter tests

8b04599

Refactor MsBibImporter tests

d3ec997

Refactor OvidImporter tests

92b3b0a

Refactor PdfXmpImporter tests

01a6328

Refactor RepecNepImporter tests

8a73a53

Refactor SilverPlatterImporter tests

c2c9341

Fix a bunch of codacy issues

2845bf3

lenhard changed the title ~~[WIP] Migrate importer tests to JUnit5~~ Migrate importer tests to JUnit5 Jan 31, 2018

lenhard added status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers and removed jabcon labels Jan 31, 2018

tobiasdiez approved these changes Jan 31, 2018

View reviewed changes

Siedlerchr merged commit 608e415 into master Jan 31, 2018

Siedlerchr deleted the migrate-parameter-tests branch January 31, 2018 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate importer tests to JUnit5 #3665

Migrate importer tests to JUnit5 #3665

lenhard commented Jan 25, 2018

tobiasdiez left a comment

lenhard commented Jan 25, 2018

tobiasdiez left a comment

Siedlerchr Jan 26, 2018

lenhard Jan 26, 2018

Siedlerchr Jan 26, 2018

lenhard Jan 26, 2018

lenhard Jan 26, 2018

Siedlerchr commented Jan 26, 2018

lenhard commented Jan 26, 2018

lenhard commented Jan 26, 2018

Siedlerchr commented Jan 26, 2018

Siedlerchr Jan 27, 2018

Siedlerchr Jan 27, 2018

Siedlerchr commented Jan 27, 2018

Siedlerchr commented Jan 30, 2018

lenhard commented Jan 31, 2018

tobiasdiez left a comment •

edited

Loading

lenhard commented Jan 31, 2018

tobiasdiez commented Jan 31, 2018

Siedlerchr commented Jan 31, 2018


		public class ImporterTestEngine {

		private static final String TEST_RESOURCES = "src/test/resources/org/jabref/logic/importer/fileformat";

Migrate importer tests to JUnit5 #3665

Migrate importer tests to JUnit5 #3665

Conversation

lenhard commented Jan 25, 2018

tobiasdiez left a comment

Choose a reason for hiding this comment

lenhard commented Jan 25, 2018

tobiasdiez left a comment

Choose a reason for hiding this comment

Siedlerchr Jan 26, 2018

Choose a reason for hiding this comment

lenhard Jan 26, 2018

Choose a reason for hiding this comment

Siedlerchr Jan 26, 2018

Choose a reason for hiding this comment

lenhard Jan 26, 2018

Choose a reason for hiding this comment

lenhard Jan 26, 2018

Choose a reason for hiding this comment

Siedlerchr commented Jan 26, 2018

lenhard commented Jan 26, 2018

lenhard commented Jan 26, 2018

Siedlerchr commented Jan 26, 2018

Siedlerchr Jan 27, 2018

Choose a reason for hiding this comment

Siedlerchr Jan 27, 2018

Choose a reason for hiding this comment

Siedlerchr commented Jan 27, 2018

Siedlerchr commented Jan 30, 2018

lenhard commented Jan 31, 2018

tobiasdiez left a comment • edited Loading

Choose a reason for hiding this comment

lenhard commented Jan 31, 2018

tobiasdiez commented Jan 31, 2018

Siedlerchr commented Jan 31, 2018

tobiasdiez left a comment •

edited

Loading