Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue 3959: migrate all tests to JUnit 5 #4260

Merged
merged 76 commits into from Aug 16, 2018

Conversation

Metatronwings
Copy link
Contributor

@Metatronwings Metatronwings commented Aug 12, 2018

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.
image

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,
image
this is the old version, and
image
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.
image

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?)

1160300305 and others added 30 commits Aug 6, 2018
*AuthorListParameterTest.java
*XmpUtilWriterTest.java
*XmpUtilReaderTest.java
*SearchQueryHighlightObservableTest.java

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

    -When running tests, outputs in command line are different. I don't know if this change is correct.
*AuthorAndToSemicolonReplacerTest.java
*FileDialogConfigurationTest.java
*IntegrityCheckTest.java
*ModsExportFormatTest.java
*AutoSetFileLinksUtilTest.java
*MsBibExportFormatTest.java
*CsvExportFormatTest.java
*HtmlExportFormatTest.java
*HtmlExportFormatTest.java
*RenamePdfCleanupTest.java
*ProtectedTermsListTest.java
*MoveFilesCleanupTest.java
    *BibTeXMLExporterTestFiles.java
    *ModsExportFormatTestFiles.java
    *MSBibExportFormatTestFiles.java
@Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 14, 2018

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
Copy link
Member

@tobiasdiez tobiasdiez commented Aug 14, 2018

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
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 14, 2018

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

@Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 14, 2018

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
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 14, 2018

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

Siedlerchr and others added 2 commits Aug 14, 2018
    *BibTeXMLExporterTestFiles.java
    *ModsExportFormatTestFiles.java
Copy link
Member

@tobiasdiez tobiasdiez left a comment

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.

build.gradle Outdated
@@ -62,7 +62,15 @@ sourceSets {
}

resources {
srcDirs = ["src/main/java", "src/main/resources"]
srcDirs = ["src/main/resources"]
Copy link
Member

@tobiasdiez tobiasdiez Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -381,7 +382,7 @@ private void assertWrong(BibDatabaseContext context) {
createBibtexKeyPatternPreferences(),
new JournalAbbreviationRepository(new Abbreviation("IEEE Software", "IEEE SW")), true)
.checkBibtexDatabase();
assertFalse(messages.toString(), messages.isEmpty());
assertFalse(messages.isEmpty(), messages.toString());
Copy link
Member

@tobiasdiez tobiasdiez Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@Metatronwings Metatronwings Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@tobiasdiez tobiasdiez Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -106,4 +103,34 @@ public void findAssociatedFilesInNonExistingDirectoryFindsNothing() throws Excep
assertEquals(Collections.emptyList(), results);
}

@AfterEach
void deleteTempFiles() throws IOException{
Copy link
Member

@tobiasdiez tobiasdiez Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Metatronwings Metatronwings Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tobiasdiez tobiasdiez Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

@tobiasdiez tobiasdiez Aug 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here also, is this really necessary?

Copy link
Contributor Author

@Metatronwings Metatronwings Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Metatronwings 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
Copy link
Member

@tobiasdiez tobiasdiez left a comment

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

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

@Siedlerchr Siedlerchr Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@Metatronwings Metatronwings Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Path path = testFolder.resolve("toot.tmp");
Files.createFile(path);
File tempFile = path.toFile();

LinkedFile fileField = new LinkedFile("", tempFile.getAbsolutePath(), "");
Copy link
Member

@Siedlerchr Siedlerchr Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public final void testPerformExport() throws IOException, SaveException {
@ParameterizedTest
@MethodSource("fileNames")
public final void testPerformExport(String filename) throws IOException, SaveException {
String xmlFileName = filename.replace(".bib", ".xml");
Path importFile = resourceDir.resolve(filename);
String tempFilename = tempFile.getCanonicalPath();
Copy link
Member

@Siedlerchr Siedlerchr Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path has a method getFileName which you should use here

Copy link
Contributor Author

@Metatronwings Metatronwings Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
image
So I guess if it is better to rename this variable to something like tempFilePath ? And I've done that in the commit.

Copy link
Member

@Siedlerchr Siedlerchr left a comment

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
Copy link
Contributor Author

@Metatronwings Metatronwings commented Aug 15, 2018

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
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 15, 2018

@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 ;)

@Metatronwings 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
@Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Aug 15, 2018

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
Copy link
Contributor Author

@Metatronwings Metatronwings commented Aug 15, 2018

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

@tobiasdiez tobiasdiez merged commit a880add into JabRef:master Aug 16, 2018
2 of 3 checks passed
@Metatronwings Metatronwings deleted the fix-for-issue-3959 branch Aug 16, 2018
Siedlerchr added a commit that referenced this issue Aug 17, 2018
* 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
@koppor koppor mentioned this pull request Oct 9, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants