Fix for issue 3959: migrate all tests to JUnit 5 by Metatronwings · Pull Request #4260 · JabRef/jabref

Metatronwings · 2018-08-12T02:29:06Z

Hi, I'm trying to fix the issue #3959 , and I have some problems here. Can someone help me?

1. Some tests don't seem to have a parameter source
Such as BibTeXMLExporterTest.java, MSBibExportFormatTestFiles.java, ModsExportFormatTestFiles.java, and so on. Since I can't get feedback from both Travis CI and my IDE, I couldn't do anything at the moment.

2. Something about the external libraries
For MainArchitectureTestsWithArchUnit.java,
testCompile 'com.tngtech.archunit:archunit-junit:0.8.3' isn't enough for migrate this test to JUnit 5.
Just a few days ago, ArchUnit just supported tests with only JUnit 5 package. But this feature needs a testCompile 'com.tngtech.archunit:archunit-junit:0.9.0 and as far as I know, this version has not been released yet.
For CiteKeyBasedFileFinderTest.java, I haven't found an extension in JUnit 5 for the MockitoJUnitRunner.class in JUnit 4. And it seems that their (mock) official version hasn't been release yet, too.

3. The Database Test
As I can see, those tests have already been migrated partly to JUnit 5. But the rest of it is the PROBLEM.
I've tried to migrate DBMSProcessorTest.java , but the code is no longer concise after that.
For example,

this is the old version, and

that is the new version. And that is only part of it.
The reason is that I couldn't pass the parameters to the @BeforeEach method in JUnit 5, but JUnit 4 can use those local variable to pass the parameters. I think this is where the difference occurred.

4. The GUI Test
I just change the @Category annotation to @Tag annotation, as I have known that those tests are mostly obsolete.

5. About CleanupWorkerTest
This test is not hard to migrate from JUnit 4 to 5. But when I finish, and run the tests on my IDE, two tests failed. And the error message is quite weird. It seems that the worker doesn't work in JUnit 5 tests.

And if there are any errors or something can be improved in other tests that I didn't mention, please let me know! Thanks!

Change in CHANGELOG.md described
Tests created for changes
Manually tested changed features in running JabRef
Screenshots added in PR description (for bigger UI changes)
Ensured that the git commit message is a good one
Check documentation status (Issue created for outdated help page at help.jabref.org?)

*AuthorListParameterTest.java

*XmpUtilWriterTest.java

*XmpUtilReaderTest.java

*SearchQueryHighlightObservableTest.java -did not use MockitoExtension.class as its official version has not been released yet.

*CitationEntryTest.java

*RTFCharsTest.java

*ProtectedTermsLoaderTest.java -When running tests, outputs in command line are different. I don't know if this change is correct.

*AuthorAndToSemicolonReplacerTest.java

…/jabref

*FileDialogConfigurationTest.java

*IntegrityCheckTest.java

*ModsExportFormatTest.java

*AutoSetFileLinksUtilTest.java

*MsBibExportFormatTest.java

*CsvExportFormatTest.java

*HtmlExportFormatTest.java

*RenamePdfCleanupTest.java

*ProtectedTermsListTest.java

*MoveFilesCleanupTest.java

*ImporterTest.java

*BibTeXMLExporterTestFiles.java *ModsExportFormatTestFiles.java *MSBibExportFormatTestFiles.java

Siedlerchr · 2018-08-14T12:47:32Z

I remember that we had problems with this before, that some resources were not int he right folder. I will take a look at that.

tobiasdiez · 2018-08-14T13:01:17Z

That now some of the exporter tests fail, were my fear. This probably needs further investigations and fixes for the exporter; thus it goes beyond the current PR. Thus, I propose to mark the tests as ignored/disabled for now and we will come back later to fix them.

Siedlerchr · 2018-08-14T13:17:51Z

I am just investigating this and I think I found the issue with the paths

Siedlerchr · 2018-08-14T13:49:13Z

It's a classpath issue, for exmaple:

        Path path = Paths.get(MSBibExportFormatTestFiles.class.getResource("").toURI());
 => Returns E:\workspace\JabRef\jabref\bin\main\org\jabref\logic\exporter

although it should be bin/test

Siedlerchr · 2018-08-14T15:18:51Z

Okay, I found the issue with the paths. I really wondered if it ever worked... But well, now it does. The key is to point to one existing resource. The rest can be found dynamically.

And this leads us actually to some failing tests. But I would ignore that for the moment. That is a difference.

@Metatronwings You can use this patch as a start for all the related importer/exporter tests.

0001-fix-resolving-of-paths-in-msbibexporterpath.patch.txt

…resource dirs in gradle

*BibTeXMLExporterTestFiles.java *ModsExportFormatTestFiles.java

tobiasdiez

Thanks again for your massive work! I went through the changes and only have two small suggestions. Please disable the failing exporter tests and then we can merge.

tobiasdiez · 2018-08-14T16:47:17Z


        resources {
-            srcDirs = ["src/main/java", "src/main/resources"]
+            srcDirs = ["src/main/resources"]


"src/main/java" should still be there since otherwise the fxml files in the src folder are not found

tobiasdiez · 2018-08-14T17:04:58Z

                new JournalAbbreviationRepository(new Abbreviation("IEEE Software", "IEEE SW")), true)
                .checkBibtexDatabase();
-        assertFalse(messages.toString(), messages.isEmpty());
+        assertFalse(messages.isEmpty(), messages.toString());


Its always better to compare values to empty collections or strings then to use assertFalse(...isEmpty()) because you get better error message in case the test fails. Of course, you didn't write this code but please change it to assertEquals("", messages).

Hmmm... The tests failed when I changed it to assertEquals("", messages).
Is that correct?

Damn...ok leave it like it was before, but this is something we also have to fix in the future...

tobiasdiez · 2018-08-14T17:09:11Z

    }

+    @AfterEach
+    void deleteTempFiles() throws IOException{


Is this really necessary? Usually the TempDirectory extension takes care of file deletion.

I think rootDir = temporaryFolder.getParent(); is the reason why those files didn't disappear automatically.
In fact, I can just use deleteIfExists(rootDir); to solve this problem, but the temp folder on my machine has some other stuff that can't be deleted (they share one temp file folder). So I have to use this to guarantee the tests run in a correct environment.

Good observation. In fact, you should never use the parent of the temporary folder, since a user might not even have write access to it. Can you please change the tests (here and below) so that they don't use the parent. Thanks.

tobiasdiez · 2018-08-14T17:11:40Z

-        File testFile = temporaryFolder.newFile(name);
-        Files.write(testFile.toPath(), name.getBytes(StandardCharsets.UTF_8), StandardOpenOption.APPEND);
-        return testFile.toPath();
+    @AfterEach void delete() throws IOException{


here also, is this really necessary?

And yes, those folders didn't delete themselves automatically. Every time I run this test, I'll get something like tens of temp folders.

-You can see in the code. :)

tobiasdiez

Thanks again for your contribution and the many quick follow-ups!

Siedlerchr · 2018-08-15T09:27:29Z

+        Path path = bibFolder.resolve("AnotherRandomlyNamedFolder");
+        Files.createDirectory(path);
+        File subfolder = path.toFile();
        File fileBefore = new File(subfolder, "test.pdf");


Just one more minor thing I have before we can merge.
Please use the nio methods here and in the other things as well, e.g. instead of the old File = new File use
Files.createFile(Path path)

Are there other tests that have this issue? Or just change this test?

Siedlerchr · 2018-08-15T09:29:30Z

+        Files.createFile(path);
+        File tempFile = path.toFile();
+
        LinkedFile fileField = new LinkedFile("", tempFile.getAbsolutePath(), "");


The path class also has a method toAbsolutePath which you can use here., e.g. just toAbsolutePath().toString()

Siedlerchr · 2018-08-15T09:31:10Z

+    public final void testPerformExport(String filename) throws IOException, SaveException {
        String xmlFileName = filename.replace(".bib", ".xml");
        Path importFile = resourceDir.resolve(filename);
        String tempFilename = tempFile.getCanonicalPath();


Path has a method getFileName which you should use here

I'm afraid that is a naming error. The tempFilename doesn't represent "filename" but "an absolute path". And you can see that in the pic below. I print tempFilename to the console.

So I guess if it is better to rename this variable to something like tempFilePath ? And I've done that in the commit.

Siedlerchr

Thanks again for your contribution! 👍 This is a huge step forward for us.
If you could just fix the path related stuff I would be happy as well and we can merge :)

-Use the NIO methods in MoveFilesCleanupTest.java -Use toAbsolutePath() method in RenamePdfCleanupTest.java -Rename "tempFilename" to "tempFilePath" in BibTeXMLExporterTestFiles.java

Metatronwings · 2018-08-15T14:51:45Z

I'm very glad to see this issue will be resolved by us soon. Only a few more small questions:

1. Some tests have not been migrated yet.
The DBMSSynchronizerTest.java and SynchronizationTestSimulator.java have not been migrated yet as there is no good way to handle chaos code after migration. (See example in DBMSProcessorTest.java)
Do I need to migrate them?
The MainArchitectureTestsWithArchUnit.java have not, too. As there is not yet a release of the new archunit.

2. Since I didn't migrate them, how should I write the CHANGELOG.md ?

And thanks for all of you! 👍

Siedlerchr · 2018-08-15T15:24:17Z

@Metatronwings It's okay, we will sure find a way for them in the long run. Not that important.
You don't need to add a changelog entry, as this is only a code-quality issue which will not have any impact on the normal JabRef user.

We hope you enjoyed it and if you want you can also further contribute. Looking to see more from you ;)

Siedlerchr · 2018-08-15T15:50:50Z

I just converted the two other classes with the legacy file methods to use the nio stuff.
0001-include-main-java-in-resources-as-well.patch.txt

Metatronwings · 2018-08-15T15:59:00Z

Sorry, but I must do that tomorrow morning because it's 12:00p.m. in my timezone. Thanks for the patch!

…o nio

* upstream/master: Fix for issue 3959: migrate all tests to JUnit 5 (#4260) Fix "Manage journal abbreviations" dialog (#4263) # Conflicts: # src/test/java/org/jabref/logic/cleanup/MoveFilesCleanupTest.java # src/test/java/org/jabref/logic/cleanup/RenamePdfCleanupTest.java

1160300305 and others added 30 commits August 6, 2018 11:24

Update migrate from JUnit 4 to JUnit 5

5d73b71

Update migrate from JUnit 4 to JUnit 5

cb1b75f

Trying to change git accounts on my laptop

45fa771

Trying to change git accounts on my laptop

ddc898d

Trying to change git accounts on my laptop

553c7b4

Using static assert methods.

f481f04

Migrate from JUnit 4 to JUnit 5

897131d

*AuthorListParameterTest.java

Migrate from JUnit 4 to JUnit 5

1e0c89e

*XmpUtilWriterTest.java

Migrate from JUnit 4 to JUnit 5

1edf14f

*XmpUtilReaderTest.java

Migrate from JUnit 4 to JUnit 5

5a34921

*SearchQueryHighlightObservableTest.java -did not use MockitoExtension.class as its official version has not been released yet.

Migrate from JUnit 4 to JUnit 5

d23dc9c

*CitationEntryTest.java

Migrate from JUnit 4 to JUnit 5

35e7621

*RTFCharsTest.java

Migrate from JUnit 4 to JUnit 5

1c4022e

*ProtectedTermsLoaderTest.java -When running tests, outputs in command line are different. I don't know if this change is correct.

Trying to change to a new branch.

2e63dc1

commit_test

959fd13

delete my meaningless txt

355a71d

Migrate from JUnit 4 to JUnit 5

9fc98e7

*AuthorAndToSemicolonReplacerTest.java

Merge branch 'fix-for-issue-3959' of https://github.com/Metatronwings…

3533b6b

…/jabref

Migrate from JUnit 4 to JUnit 5

f74f486

*FileDialogConfigurationTest.java

Migrate from JUnit 4 to JUnit 5

a1df42f

*IntegrityCheckTest.java

Migrate from JUnit 4 to JUnit 5

970502a

*ModsExportFormatTest.java

Migrate from JUnit 4 to JUnit 5

b18c9ba

*AutoSetFileLinksUtilTest.java

Migrate from JUnit 4 to JUnit 5

fb157f4

*MsBibExportFormatTest.java

Migrate from JUnit 4 to JUnit 5

7761b29

*CsvExportFormatTest.java

Migrate from JUnit 4 to JUnit 5

aeee7ef

*HtmlExportFormatTest.java

Migrate from JUnit 4 to JUnit 5

119f491

*HtmlExportFormatTest.java

Migrate from JUnit 4 to JUnit 5

dbf6341

*RenamePdfCleanupTest.java

Migrate from JUnit 4 to JUnit 5

8d3c78e

*ProtectedTermsListTest.java

Migrate from JUnit 4 to JUnit 5

21e539a

*MoveFilesCleanupTest.java

Migrate from JUnit 4 to JUnit 5

3b0c544

*ImporterTest.java

Change the slash direction.

4fb0ef0

*BibTeXMLExporterTestFiles.java *ModsExportFormatTestFiles.java *MSBibExportFormatTestFiles.java

Siedlerchr and others added 2 commits August 14, 2018 23:25

fix resolving of paths in msbibexporterpath update junit and correct …

fb925e5

…resource dirs in gradle

Fix the path issue in

f46980d

*BibTeXMLExporterTestFiles.java *ModsExportFormatTestFiles.java

tobiasdiez requested changes Aug 14, 2018

View reviewed changes

Disable the failing exporter tests and change the build.gradle

5e4324f

Metatronwings changed the title ~~(need help) [WIP] Fix for issue 3959: migrate all tests to JUnit 5~~ (almost done) [WIP] Fix for issue 3959: migrate all tests to JUnit 5 Aug 15, 2018

Metatronwings added 2 commits August 15, 2018 11:39

Merge remote-tracking branch 'upstream/master'

3daf305

Fix the parent folder issue by make every path to their child once.

25030de

-You can see in the code. :)

tobiasdiez approved these changes Aug 15, 2018

View reviewed changes

Siedlerchr reviewed Aug 15, 2018

View reviewed changes

Fix some minor issues.

a81767a

-Use the NIO methods in MoveFilesCleanupTest.java -Use toAbsolutePath() method in RenamePdfCleanupTest.java -Rename "tempFilename" to "tempFilePath" in BibTeXMLExporterTestFiles.java

Metatronwings changed the title ~~(almost done) [WIP] Fix for issue 3959: migrate all tests to JUnit 5~~ Fix for issue 3959: migrate all tests to JUnit 5 Aug 15, 2018

include main/java in resources as well convert legalcy file methods t…

3deb4f8

…o nio

tobiasdiez merged commit a880add into JabRef:master Aug 16, 2018

tobiasdiez mentioned this pull request Aug 16, 2018

Migrate all tests to JUnit 5 #3959

Closed

Metatronwings deleted the fix-for-issue-3959 branch August 16, 2018 09:47

koppor mentioned this pull request Oct 9, 2019

Convert remaining tests to junit 5 #5394

Merged

6 tasks

Uh oh!

Conversation

Metatronwings commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siedlerchr commented Aug 14, 2018

Uh oh!

tobiasdiez commented Aug 14, 2018

Uh oh!

Siedlerchr commented Aug 14, 2018

Uh oh!

Siedlerchr commented Aug 14, 2018

Uh oh!

Siedlerchr commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tobiasdiez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Siedlerchr left a comment

Choose a reason for hiding this comment

Uh oh!

Metatronwings commented Aug 15, 2018

Uh oh!

Siedlerchr commented Aug 15, 2018

Uh oh!

Siedlerchr commented Aug 15, 2018

Uh oh!

Metatronwings commented Aug 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Metatronwings commented Aug 12, 2018 •

edited

Loading

Siedlerchr commented Aug 14, 2018 •

edited

Loading