Add simple Unit Tests #7542

Davfon · 2021-03-15T18:53:25Z

I have added some simple unit tests that will increase code coverage / branch coverage.
They contribute to issue #6207

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

calixtus · 2021-03-15T20:50:53Z

Hey @Davfon , great to see someone takes a look at our test suite!
At first glance, the tests themselves look good. But I think, you could also convert these tests to parameterized tests (there are some examples in our test suite) if you like. It's pretty easy. You could also use the builder pattern for the creation of bibentries for testing (new BibEntry().withField()...)

Davfon · 2021-03-16T11:19:33Z

Hi there @calixtus !

Thanks for the quick feedback! I willl look into it and will try to convert them to parameterized tests wherever possible.

calixtus · 2021-03-17T22:39:41Z

src/test/java/org/jabref/model/entry/MonthTest.java

@@ -151,4 +153,163 @@ public void parseCorrectlyByShortNameGermanLowercase() {
        assertEquals(Optional.of(Month.NOVEMBER), Month.parse("nov"));
        assertEquals(Optional.of(Month.DECEMBER), Month.parse("dez"));
    }
+
+    @Test
+    public void parseGermanShortMonthTest() {


Is there more potential for parameterized tests here?

Yes, there is. Thanks for the hint. I wasn't aware that you can also pass the expected result to the test.
Really nice feature :D
I changed all of them to parameterized Tests now.

Siedlerchr · 2021-03-19T21:07:38Z

src/test/java/org/jabref/model/texparser/CitationTest.java

+
+    @Test
+    public void hashCodeTest() {
+        assertEquals(Objects.hash(path, 10, 1, 4, "lineText"), citation.hashCode());


Tests for Hashcode and toString do not really add any value and especially toString is more of an internal representation.

Oh ok, seemed like a rather easy way to increase the code coverage. Thanks for letting me know, I'll try to avoid them in them in the future. (I removed them with commit 21050ff)

Siedlerchr · 2021-03-19T21:08:37Z

src/main/java/org/jabref/model/entry/Month.java

@@ -110,7 +110,7 @@
     * @return the corresponding month instance, empty if input is not in German
     *         form
     */
-    private static Optional<Month> parseGermanShortMonth(String value) {
+    public static Optional<Month> parseGermanShortMonth(String value) {


Maybe just remove the public prefix and make it "default"

I'm not sure I understand it correctly, you mean like this: 5022f10?

Yes, this makes it package visible, so it can be accessed by the unit test but not from others https://www.baeldung.com/java-access-modifiers

ellieMayVelasquez · 2021-03-21T17:30:23Z

src/test/java/org/jabref/logic/integrity/CitationKeyCheckerTest.java

        assertEquals(Collections.emptyList(), checker.check(entry));
    }

+    @Test
+    void KeyFromAuthorAndTitle() {


I detect that this code is problematic. According to the Bad practice (BAD_PRACTICE), Nm: Method names should start with a lower case letter (NM_METHOD_NAMING_CONVENTION).
Methods should be verbs, in mixed case with the first letter lowercase, with the first letter of each internal word capitalized.

Thanks for the feedback! Changed it in commit 93ab8a4

Happy to help.

Siedlerchr · 2021-03-24T19:03:51Z

src/test/java/org/jabref/model/texparser/CitationTest.java

+    public void equalsTest() {
+        Citation citation1 = new Citation(path, 10, 1, 4, "lineText");
+        Citation citation2 = null;
+        assertTrue(citation.equals(citation1));


This does not make any sense, why don't you use assertEquals? assertEquals calls the equals of the underlying objects. There is also a assertNotEquals method

Yeah, that one is on me. Thanks for the feedback! :) Fixed in commit 6195973

Siedlerchr

Just some minor issue, rest is okay

calixtus

Looks good to me too, thanks! Merging now.

* upstream/master: (191 commits) Fix for issue 7416: font size of the preferences dialog does not update with the rest of the GUI. (#7509) Fix school/instituation is printed twice (#7574) Dsiable notarisation until we hae an account for JabRef e.V. (#7572) Fix citation keys unintentionally being overwritten on import (#7443) Fix AuthentificationPlugin not declared in mergedModule (#7570) Suggestions for changes in caching latex free authors (#7301) Add simple Unit Tests (#7542) Fix drag and drop into empty library (#7555) Bump richtextfx from 0.10.4 to 0.10.6 (#7563) Bump pdfbox from 2.0.22 to 2.0.23 (#7561) Bump org.eclipse.jgit (#7560) Bump fontbox from 2.0.22 to 2.0.23 (#7562) Bump guava from 30.1-jre to 30.1.1-jre (#7564) Bump xmpbox from 2.0.22 to 2.0.23 (#7565) Bump hmarr/auto-approve-action from v2.0.0 to v2.1.0 (#7566) Add gource (#7193) UI: Fix for group icon (#7552) Fix for issue 6487: Opening BibTex file (doubleclick) from Folder with spaces not working (#7551) add ability to insert arxivId (#7549) Fixed missing trigger for linked file operations (#7548) ...

Davfon added 13 commits March 12, 2021 16:16

#6207 Add simple Unit Tests (MonthTest.java)

06763bc

MonthTest: formatting

5416e32

Merge remote-tracking branch 'upstream/master' into a1-df

5c0b6ea

#6207 Add simple Unit Tests (CitationTest.java)

69c3142

#6207 Add simple Unit Tests (DateCheckerTest.java)

b670f48

DateCheckerTest: consider different date formats (partitions)

3d97c85

CitationTest: consider boundaries

f44c59c

BracketCheckerTest: consider different types of input (partitions)

1b20571

CitationKeyCheckerTest: consider different types of input (partitions)

8fcda4d

DoiValidityCheckerTest: consider boundary cases

3a00cc6

CitationKeyCheckerTest: rename tests

e0e451a

StringLengthComparatorTest: consider boundary cases

21a5193

Merge remote-tracking branch 'upstream/master' into a1-df

18f4e95

Convert to Parameterized Tests

d29cfea

calixtus reviewed Mar 17, 2021

View reviewed changes

MonthTest: Convert to Parameterized Tests

8e878c4

Siedlerchr reviewed Mar 19, 2021

View reviewed changes

ellieMayVelasquez reviewed Mar 21, 2021

View reviewed changes

Davfon added 4 commits March 22, 2021 11:56

CitationTest: remove hashCodeTest() & toStringTest()

21050ff

Month: remove the public prefix

5022f10

CitationKeyCheckerTest: first letter lowercase

93ab8a4

CitationTest: remove unused import

a166956

Siedlerchr approved these changes Mar 24, 2021

View reviewed changes

Siedlerchr reviewed Mar 24, 2021

View reviewed changes

Siedlerchr requested changes Mar 24, 2021

View reviewed changes

CitationTest: use assertEqual()

6195973

Siedlerchr approved these changes Mar 25, 2021

View reviewed changes

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 25, 2021

calixtus approved these changes Mar 25, 2021

View reviewed changes

calixtus merged commit 49b9e3c into JabRef:master Mar 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple Unit Tests #7542

Add simple Unit Tests #7542

Davfon commented Mar 15, 2021 •

edited

Loading

calixtus commented Mar 15, 2021

Davfon commented Mar 16, 2021

calixtus Mar 17, 2021

Davfon Mar 19, 2021 •

edited

Loading

Siedlerchr Mar 19, 2021

Davfon Mar 22, 2021

Siedlerchr Mar 19, 2021

Davfon Mar 22, 2021

Siedlerchr Mar 22, 2021

ellieMayVelasquez Mar 21, 2021

Davfon Mar 22, 2021

ellieMayVelasquez Mar 22, 2021

Siedlerchr Mar 24, 2021

Davfon Mar 24, 2021 •

edited

Loading

Siedlerchr left a comment

calixtus left a comment

Add simple Unit Tests #7542

Add simple Unit Tests #7542

Conversation

Davfon commented Mar 15, 2021 • edited Loading

calixtus commented Mar 15, 2021

Davfon commented Mar 16, 2021

Choose a reason for hiding this comment

Davfon Mar 19, 2021 • edited Loading

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

Davfon Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

calixtus left a comment

Choose a reason for hiding this comment

Davfon commented Mar 15, 2021 •

edited

Loading

Davfon Mar 19, 2021 •

edited

Loading

Davfon Mar 24, 2021 •

edited

Loading