Refactoring and addition of unit tests #7581

BShaq · 2021-03-28T12:06:58Z

This pull request includes new unit tests for three classes which increases their line/statement coverage.
They contribute to issue #6207.

Further, I have also refactored existing unit tests to make them more readable, understandable and maintanable.
In this sense, I focused more on test smells described in the paper Refactoring test code (van Deursen et al.) and also on the Code Howtos (Test Cases) in the Development Documentation.

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.

Added assertion explanation to give an explanatory message when the assertion fails.

Additionally removed the setup() method, since it was only used by a few test cases

Tests which exercise more than one production method are harder to read and understand, thus difficult to use as documentation. By splitting the two refactored tests into multiple ones, the readability has hugely increased.

small fix

… Eager Test) To not introduce another Test Smell (Sensitive Equality), I used the equals() method to compare the strings.

…Tests) Additionally removed the 'test' prefix in the naming as suggested in the 'Code Howtos' of the contribution guidelines.

…nsitive Equality) In addition, I removed the 'test' prefix in the namings as suggested in the contribution guidelines.

Added description for each assertion, which makes it easier to detect which one failed (in the case when the test itself fails)

Created new unit tests for each production method.

Increase of Line/Statement coverage

koppor

Some comments, especially on using assertEquals(Optional.of(...), ...) instead of assertTrue(...isPresent()

src/test/java/org/jabref/cli/JabRefCLITest.java

koppor · 2021-04-12T20:50:05Z

src/test/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationViewModelTest.java

+    @Test
+    public void sameAuthor() {
+        String expectedAuthor = "Jaroslav Kucha ˇr";
+        assertTrue(annotationViewModel.getAuthor().equals(expectedAuthor));


Why not assertEquals? This is the usual way to check .equals.

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

src/test/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationViewModelTest.java

+        String expectedPage = "1";
+        assertTrue(annotationViewModel.getPage().equals(expectedPage));


Suggested change

String expectedPage = "1";

assertTrue(annotationViewModel.getPage().equals(expectedPage));

assertEquals("1", annotationViewModel.getPage());

koppor · 2021-04-12T20:51:05Z

src/test/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationViewModelTest.java

+    @Test
+    public void retrieveCorrectDateAsString() {
+        String expectedDate = "2017-07-20 10:11:30";
+        assertTrue(annotationViewModel.getDate().equals(expectedDate));


Please create a one-liner like shown above.

koppor · 2021-04-12T20:51:09Z

src/test/java/org/jabref/gui/entryeditor/fileannotationtab/FileAnnotationViewModelTest.java

+    @Test
+    public void retrieveCorrectContent() {
+        String expectedContent = "This is content";
+        assertTrue(annotationViewModel.getContent().equals(expectedContent));


Please create a one-liner like shown above.

koppor · 2021-04-12T20:53:59Z

src/test/java/org/jabref/logic/bibtex/comparator/EntryComparatorTest.java

+
+    @Test
+    void compareAuthorFieldBiggerAscending() throws Exception {
+


No starting empty line please

koppor · 2021-04-12T20:54:30Z

src/test/java/org/jabref/logic/bibtex/comparator/EntryComparatorTest.java

+    private BibEntry entry1;
+    private BibEntry entry2;
+
+    @BeforeEach
+    void setup() {
+        entry1 = new BibEntry();
+        entry2 = new BibEntry();
+    }
+


Suggested change

private BibEntry entry1;

private BibEntry entry2;

@BeforeEach

void setup() {

entry1 = new BibEntry();

entry2 = new BibEntry();

}

private BibEntry entry1 = new BibEntry();

private BibEntry entry2 = new BibEntry();

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

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

    }

+    private EditionChecker createBibtexEditionChecker(Boolean bool) {


Please provide a speaking name for bool

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

src/test/java/org/jabref/logic/util/UpdateFieldTest.java

+        BibEntry entry2 = new BibEntry();
+        entry2.setField(StandardField.OWNER, initialOwner);
+
+        assertTrue(entry.getField(StandardField.OWNER).isPresent(), "Owner field for entry is not present");


Suggested change

assertTrue(entry.getField(StandardField.OWNER).isPresent(), "Owner field for entry is not present");

assertEquals(Optional.of(initialOwner), entry.getField(StandardField.OWNER));

Similar for all other Optional tests. We should avoid assertTrue in all cases; we should use assertEquals wherevery possible.

koppor · 2021-04-12T20:57:07Z

src/test/java/org/jabref/logic/util/UpdateFieldTest.java

+        TimestampPreferences timestampPreferences = createTimestampPreference();
+        UpdateField.setAutomaticFields(bibs, false, ownerPreferences, timestampPreferences);
+
+        assertEquals(entry.getField(StandardField.OWNER).get(), initialOwner, "entry has new value for owner field");


Suggested change

assertEquals(entry.getField(StandardField.OWNER).get(), initialOwner, "entry has new value for owner field");

assertEquals(Optional.of(initalOwner), entry.getField(StandardField.OWNER));

Was accidentally removed.

…Test also created one-liner as requested

BShaq · 2021-04-26T16:57:46Z

Hi @koppor
Thanks for reviewing!

I finally adjusted the code accoring to your comments and suggestions :)

koppor · 2021-05-06T21:03:53Z

src/test/java/org/jabref/gui/util/TooltipTextUtilTest.java

-    public void testCreateTextBold() {
-        String testText = "this is a test text";
+    public void stringRemainsTheSameAfterTransformationToNormal() {
+        Text text = TooltipTextUtil.createText(testText, TooltipTextUtil.TextType.NORMAL);


Not addressed. Not that important, but this

String textStyle = "Regular"; assertEquals(textStyle, text.getFont().getStyle());

is IMHO less readable than

assertEquals("Regular", text.getFont().getStyle());

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

BShaq added 20 commits March 24, 2021 13:53

Resolved Test Smell "Assertion Roulette"

50e89ff

Added assertion explanation to give an explanatory message when the assertion fails.

Reduced Test Code Duplication (Test Smell)

0ec8a52

Reduced Test Code Duplication (Test Smell) for EditionCheckerTest.java

9a3ec82

Additionally removed the setup() method, since it was only used by a few test cases

Update BibDatabaseTest.java

14f149e

small fix

Refactoring of FileAnnotationViewModelTest.java (resolved Test Smell:…

8add785

… Eager Test) To not introduce another Test Smell (Sensitive Equality), I used the equals() method to compare the strings.

Refactored MainTableColumnModelTest.java (resolved Test Smell: Eager …

e4b183d

…Tests) Additionally removed the 'test' prefix in the naming as suggested in the 'Code Howtos' of the contribution guidelines.

Refactored TooltipTextUtil.java (resolved Test Smells: Eager Test, Se…

5c64b84

…nsitive Equality) In addition, I removed the 'test' prefix in the namings as suggested in the contribution guidelines.

Refactored ImportDataTest.java (resolved Test Smell: Assertion Roulette

edab481

Added description for each assertion, which makes it easier to detect which one failed (in the case when the test itself fails)

Refactored JabRefCLITest.java (resolved Test Smell: Eager Tests)

74f826b

Created new unit tests for each production method.

Small fix to JabRefCLITest.java

0315466

Merge remote-tracking branch 'upstream/master' into a2-bs

7b4117c

Additional unit tests for EntryComparator.java

6be0110

Increase of Line/Statement coverage

additional unit tests for UpdateFieldTest.java

0c7467c

Increase of Line/Statement coverage

removed comments and duplicated code for EntryComparatorTest.java

4053acd

additional unit tests for BibDatabaseDiffTest.java

c772b09

Increase of Line/Statement coverage

Merge remote-tracking branch 'upstream/master' into a2-bs

b47441c

small fix on TooltipTextUtilTest.java

a0bc8f9

removed test code duplication in BibDatabaseTest.java

80f48e2

Merge remote-tracking branch 'upstream/main' into a2-bs

2e1e956

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

BShaq added 7 commits April 26, 2021 17:14

Merge remote-tracking branch 'upstream/main' into a2-bs

8622db6

added export preferences tests again

aebae90

Was accidentally removed.

exchanged assertTrue() with assertEquals() in FileAnnotationViewModel…

5a142bf

…Test also created one-liner as requested

used assertEquals() in TooltipTextUtilTest

ecdc372

minor fixes in EntryComparatorTest

3549ac7

minor fixes in EditionCheckerTest

9d9725e

further fixes in UpdateFieldTest

6bfa64c

Merge remote-tracking branch 'upstream/main' into a2-bs

58232ff

koppor approved these changes May 6, 2021

View reviewed changes

koppor merged commit 65874f5 into JabRef:main May 6, 2021

koppor removed the status: changes required Pull requests that are not yet complete label May 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring and addition of unit tests #7581

Refactoring and addition of unit tests #7581

BShaq commented Mar 28, 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

BShaq commented Apr 26, 2021

koppor May 6, 2021

		String expectedPage = "1";
		assertTrue(annotationViewModel.getPage().equals(expectedPage));

	String expectedPage = "1";
	assertTrue(annotationViewModel.getPage().equals(expectedPage));
	assertEquals("1", annotationViewModel.getPage());


		@Test
		void compareAuthorFieldBiggerAscending() throws Exception {

		}

		private EditionChecker createBibtexEditionChecker(Boolean bool) {

	assertTrue(entry.getField(StandardField.OWNER).isPresent(), "Owner field for entry is not present");
	assertEquals(Optional.of(initialOwner), entry.getField(StandardField.OWNER));

	assertEquals(entry.getField(StandardField.OWNER).get(), initialOwner, "entry has new value for owner field");
	assertEquals(Optional.of(initalOwner), entry.getField(StandardField.OWNER));

Refactoring and addition of unit tests #7581

Refactoring and addition of unit tests #7581

Conversation

BShaq commented Mar 28, 2021

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

BShaq commented Apr 26, 2021

Choose a reason for hiding this comment