Add simple unit tests #7543

nasdas-dev · 2021-03-15T20:19:16Z

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.

FieldChange.java 18% -> 94% Abbreviation.java 63% -> 88% SuggestionProviders.java 0% -> 100%

FileHelper.java -> Boundary testing of an empty file CitationKeyGenerator.java -> Boundary testing of testlagepage parser for 0-00 & 1-1 HTMLCharacterChecker.java -> Null Value Boundary test

ParsedEntryLink.java -> Partition testing of ParsingEntryLink UpperCaseFormatter.java -> Partition testing for special characters CitationStyleCacheTest.java -> Partition testing of cache storage

SPTest typo fix

nasdas-dev · 2021-03-16T08:49:50Z

will adjust and fix checkstyle this evening/tomorrow.

Checkstyle passed

ellieMayVelasquez · 2021-03-21T17:31:28Z

src/test/java/org/jabref/logic/journals/AbbreviationTest.java

+    @Test
+    void testEquals() {
+      Abbreviation abbreviation = new Abbreviation("Long Name", "L N", "LN");
+      assertTrue(abbreviation.equals(abbreviation));


I detect that this code is problematic. According to the Correctness (CORRECTNESS), SA: Self comparison of value with itself (SA_LOCAL_SELF_COMPARISON).
This method compares a local variable with itself, and may indicate a typo or a logic error. Make sure that you are comparing the right things.

Thanks for the feedback! This test originally was meant to cover the branch in the implementation of the equals method, where it returns true for being compared to itself. But seeing your linked references, I agree that it makes more sense to leave it out - I have adjusted the test now.

Happy to help :)

@ellieMayVelasquez

Adjustments for @ellieMayVelasquez feedback #7543 (review)

koppor

Some comments in there. The main comment is that toString doesn't need to be tested if the software does not rely on that contract.

koppor · 2021-04-12T18:45:12Z

src/test/java/org/jabref/gui/autocompleter/SuggestionProvidersTest.java

+      entry.setField(journalEntryField, "Journal");
+      entry.setField(publisherEntryField, "Publisher");
+      entry.setField(specialEntryField, "2000");
+      database.insertEntry(entry);


Why is the database constructed here? Where is it used? Maybe, the whole part can be removed?

koppor · 2021-04-12T18:47:10Z

src/test/java/org/jabref/gui/autocompleter/SuggestionProvidersTest.java

+      entry.setField(specialEntryField, "2000");
+      database.insertEntry(entry);
+
+      assertSame("org.jabref.gui.autocompleter.EmptySuggestionProvider", empty.getForField(personEntryField).getClass().getName());


Pls use assertEquals, because of consistency reasons to other tests (assertSame is seldmoly used).

Suggested change

assertSame("org.jabref.gui.autocompleter.EmptySuggestionProvider", empty.getForField(personEntryField).getClass().getName());

assertEquals(org.jabref.gui.autocompleter.EmptySuggestionProvider.class, empty.getForField(personEntryField).getClass());

(applies for all lines here)

koppor · 2021-04-12T20:37:33Z

src/test/java/org/jabref/logic/citationkeypattern/CitationKeyGeneratorTest.java

@@ -747,6 +747,9 @@ void testLastPage() {
        assertEquals("97", CitationKeyGenerator.lastPage("7,41,73--97"));
        assertEquals("97", CitationKeyGenerator.lastPage("7,41,97--73"));
        assertEquals("43", CitationKeyGenerator.lastPage("43+"));
+        assertEquals("0", CitationKeyGenerator.lastPage("00--0"));
+        assertEquals("1", CitationKeyGenerator.lastPage("1--1"));
+


Please no empty line at the end of the method

Please convert the whole test to a @ParameterizedTest. See #7544 for an inspiration

koppor · 2021-04-12T20:39:32Z

src/test/java/org/jabref/logic/citationstyle/CitationStyleCacheTest.java

+    BibEntry bibEntry = new BibEntry();
+    bibEntry.setCitationKey("test");
+    List<BibEntry> entries = new ArrayList<>();
+    entries.add(0, bibEntry);
+    BibDatabase database = new BibDatabase(entries);


Can't one do the following?

Suggested change

BibEntry bibEntry = new BibEntry();

bibEntry.setCitationKey("test");

List<BibEntry> entries = new ArrayList<>();

entries.add(0, bibEntry);

BibDatabase database = new BibDatabase(entries);

BibDatabase database = new BibDatabase(List.of(new BibEntry("test"));

koppor · 2021-04-12T20:39:56Z

src/test/java/org/jabref/logic/citationstyle/CitationStyleCacheTest.java

+    BibDatabaseContext databaseContext = new BibDatabaseContext(database);
+    CitationStyleCache csCache = new CitationStyleCache(databaseContext);
+
+    assertNotNull(csCache.getCitationFor(bibEntry));


Please test for a concrete result.

koppor · 2021-04-12T20:42:24Z

src/test/java/org/jabref/model/FieldChangeTest.java

+  @Test
+  void testHashCode() {
+    FieldChange fcNull = new FieldChange(null, null, null, null);
+    assertEquals(923521, fcNull.hashCode());
+  }


Please no test for hashCode.

You can include https://jqno.nl/equalsverifier/ as dependency and work on that. Please do not test for that manually.

koppor · 2021-04-12T20:43:11Z

src/test/java/org/jabref/model/FieldChangeTest.java

+
+  @Test
+  void testEquals() {
+    BibEntry entry = new BibEntry();


This is a very hard to read test. Please use https://jqno.nl/equalsverifier/ or just delete the test.

I never used https://jqno.nl/equalsverifier/ for myself. Please add a proposal based on that and we can investigate whether this is really worth it or causes much trouble.

koppor · 2021-04-12T20:43:43Z

src/test/java/org/jabref/model/FieldChangeTest.java

+  }
+
+  @Test
+  void testToString() {


Even though "Effective Java" recomments that toString is a contract; JabRef does not rely on that in this case. Please remove that test.

koppor · 2021-04-12T20:44:10Z

src/test/java/org/jabref/logic/journals/AbbreviationTest.java

@@ -101,4 +103,18 @@ void testDefaultAndShortestUniqueAbbreviationsAreSame() {
        Abbreviation abbreviation = new Abbreviation("Long Name", "L N");
        assertEquals(abbreviation.getAbbreviation(), abbreviation.getShortestUniqueAbbreviation());
    }
+
+    @Test
+    void testToString() {


JabRef does not rely on the toString contract in this case. Please remove the test.

If you want, you can check for nonNull of toString.

koppor · 2021-04-12T20:45:03Z

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

@@ -29,6 +31,7 @@ public void before() {
        link = links.get(0);
        source = create("source");
        target = create("target");
+        entry = create("entry");


Please do not initalize the variable here. It is used only in one test. Just move the variable to givenBibEntryWhenParsingThenExpectLink.

koppor · 2021-04-17T15:04:25Z

I think your course is over @ellieMayVelasquez, thus no more activity here?

nasdas-dev · 2021-04-18T14:39:39Z

I think your course is over @ellieMayVelasquez, thus no more activity here?

Thanks a lot for all the feedback @koppor ! - I will resolve all of these issues within this month hopefully, just haven't gotten around to it yet due to lots of parallel coursework atm :)

koppor · 2021-05-06T20:46:54Z

@nasdas-dev May I ask which month you meant? 😇

addressed all comments & suggestions to test files

* upstream/main: (71 commits) [Bot] Update CSL styles (#7735) Fix for issue 6966: open all files of multiple entries (#7709) Add simple unit tests (#7696) Add simple unit tests (#7543) Update check-outdated-dependencies.yml Added preset for new entry keybindings and reintroduced defaults (#7705) Select the entry which has smaller dictonary order when merge (#7708) Update CHANGELOG.md fix: make more fields, fomatters, ids and languages sorted by alphabetical order (#7717) Bump libreoffice from 7.1.2 to 7.1.3 (#7721) Bump unoloader from 7.1.2 to 7.1.3 (#7724) Bump org.beryx.jlink from 2.23.7 to 2.23.8 (#7723) Bump org.openjfx.javafxplugin from 0.0.9 to 0.0.10 (#7725) fix: make fields sorted by lexicographical order (#7711) Fix tests Refactoring existing unit tests (#7687) Refactoring and addition of unit tests (#7581) Refactor simple Unit Tests (#7571) Add simple unit tests (#7544) add and extend unit tests (#7685) ...

nasdas-dev added 5 commits March 15, 2021 15:12

Branch Coverage Tests

3084c57

FieldChange.java 18% -> 94% Abbreviation.java 63% -> 88% SuggestionProviders.java 0% -> 100%

Boundary Tests

0004bc5

FileHelper.java -> Boundary testing of an empty file CitationKeyGenerator.java -> Boundary testing of testlagepage parser for 0-00 & 1-1 HTMLCharacterChecker.java -> Null Value Boundary test

Partition Tests

4e4f8fe

ParsedEntryLink.java -> Partition testing of ParsingEntryLink UpperCaseFormatter.java -> Partition testing for special characters CitationStyleCacheTest.java -> Partition testing of cache storage

Update SuggestionProvidersTest.java

8f68850

SPTest typo fix

upstream fetch

f8ed4b1

CodeStyle Fixes

4ea42ed

Checkstyle passed

ellieMayVelasquez reviewed Mar 21, 2021

View reviewed changes

Update AbbreviationTest.java

8e297b7

Adjustments for @ellieMayVelasquez feedback #7543 (review)

koppor requested changes Apr 12, 2021

View reviewed changes

koppor added the status: changes required Pull requests that are not yet complete label Apr 12, 2021

nasdas-dev added 2 commits May 9, 2021 20:26

Applying feedback to PR

4f7c6ba

addressed all comments & suggestions to test files

checkstyle fix

2723e8e

Siedlerchr approved these changes May 11, 2021

View reviewed changes

Siedlerchr merged commit 807f85e into JabRef:main May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple unit tests #7543

Add simple unit tests #7543

nasdas-dev commented Mar 15, 2021 •

edited

Loading

nasdas-dev commented Mar 16, 2021

ellieMayVelasquez Mar 21, 2021

nasdas-dev Mar 26, 2021

ellieMayVelasquez Mar 26, 2021

koppor left a comment

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor Apr 12, 2021

koppor commented Apr 17, 2021

nasdas-dev commented Apr 18, 2021

koppor commented May 6, 2021

	assertSame("org.jabref.gui.autocompleter.EmptySuggestionProvider", empty.getForField(personEntryField).getClass().getName());
	assertEquals(org.jabref.gui.autocompleter.EmptySuggestionProvider.class, empty.getForField(personEntryField).getClass());

Add simple unit tests #7543

Add simple unit tests #7543

Conversation

nasdas-dev commented Mar 15, 2021 • edited Loading

nasdas-dev commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor left a 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

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

koppor commented Apr 17, 2021

nasdas-dev commented Apr 18, 2021

koppor commented May 6, 2021

nasdas-dev commented Mar 15, 2021 •

edited

Loading