Add more unit tests to three gui classes #7636

ningxie1991 · 2021-04-16T19:38:31Z

This pull request replaces #7635 (closed/not merged), and it contributes to issue #6207, which is to add more unit tests to the project, in this case specifically in the gui package using test doubles.

Tests added:

NewEntryActionTest
CopyMoreActionTest
ExportToClipboardActionTest

In order to add the ExportToClipboardActionTest and to migrate to using PreferencesService, code refactoring has been done as suggested by @Siedlerchr. The purpose of the refactoring is to add PreferencesService as a parameter in the constructors of ExportToClipboardAction to replace the Globals variable. Refactored classes are as follows:

ExportToClipboardAction.java
JabRefFrame.java
RightClickMenu.java
JabRefPreferences.java
PreferencesService.java

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.

…ngxie1991-a3-nx * 'a3-nx' of https://github.com/ningxie1991/jabref: fix checkstyle issue and comment out the use of Globals Modified the tests modified the tests organized imports Add JUnit test with mocks for CopyMoreAction Added two Junit tests using mocks for the gui

Siedlerchr

Thanks again for the good work!

Siedlerchr · 2021-04-17T10:18:55Z

@calixtus can you take a look at the Globals.prefs extraction?

calixtus

Some questions about the tests, and some clearification about the fileDirforDatabase variable.

Extraction of Globals.prefs looks good.

calixtus · 2021-04-16T20:53:10Z

src/main/java/org/jabref/preferences/JabRefPreferences.java

+    @Override
+    public FileLinkPreferences getFileLinkPreferences() {
        return new FileLinkPreferences(
                get(MAIN_FILE_DIRECTORY), // REALLY HERE?
                fileDirForDatabase);
    }

+    @Override
+    public void storeFileDirforDatabase(List<Path> dirs) {
+        this.fileDirForDatabase = dirs;
+    }
+


getFileLinkPreferences needs to be removed in the future as well as the variable fileDirForDatabase, since this is some pretty dirty hack from earlier times in JabRef that needs to be done right sometime. So changing something here is lost effort. Furthermore storeFileDirforDatabase does not persistently store a variable, therefor the naming scheme get/store should not be used here.

Maybe this variable is better located in the stateManager? @tobiasdiez

calixtus · 2021-04-16T20:54:07Z

src/main/java/org/jabref/preferences/PreferencesService.java

+    FileLinkPreferences getFileLinkPreferences();
+
+    void storeFileDirforDatabase(List<Path> dirs);
+


please remove this

Then I canot set this https://github.com/JabRef/jabref/pull/7636/files#diff-c9bd7e38f9d2f4a7eff486f337db736d6108369bb482c194ca8281ccfd61b93aL105

Yes. This setter is no fix for the hack, but a disguise.

Keep it ugly, until it's fixed. 😏

Sure I would, but it needs to be present in the preferences Service, because we need to get rid of the Globals for the test, as we cannot mock it.

I would vote to create a follow up PR to fix this

Yes, a follow-up is most urgent needed, but there was a reason for that hack, I'm afraid, there is no easy fix for this. See declaration of this:

// The following field is used as a global variable during the export of a database. // By setting this field to the path of the database's default file directory, formatters // that should resolve external file paths can access this field. This is an ugly hack // to solve the problem of formatters not having access to any context except for the // string to be formatted and possible formatter arguments. public List<Path> fileDirForDatabase;

But to say that again with most deep concern: The proposed change, to put these two methods into the preferencesService shall not happen!

After careful consideration: @Siedlerchr and me think it would be right, to keep the changes as SiedlerChr added, since extracting this mess of a temp variable in the preferences totally exploding this PR. This calls for a distinct one. Good luck, @Siedlerchr 😆

calixtus · 2021-04-16T20:56:45Z

src/test/java/org/jabref/gui/edit/CopyMoreActionTest.java

+        copyMoreAction.execute();
+
+        verify(clipBoardManager, times(0)).setContent(any(String.class));
+        verify(dialogService, times(1)).notify(Localization.lang("None of the selected entries have titles."));


I thought I remember that junit provides something like any() as an argument replacement, since dealing with translations in tests that are not especially tailored for language test can mess things quickly up.

calixtus · 2021-04-16T20:57:47Z

src/test/java/org/jabref/gui/edit/CopyMoreActionTest.java

+
+        String copiedTitles = String.join("\n", titles);
+        verify(clipBoardManager, times(1)).setContent(copiedTitles);
+        verify(dialogService, times(1)).notify(Localization.lang("Warning: %0 out of %1 entries have undefined title.",


Same here: can the localized argument be exchanged with something generic?
There are more below...

Yes, they can by just any() but because each test is testing for a specific condition, for example when no entries have titles, or when some entries have titles. In different cases, the diaglogService notitfy shows different corresponding messages. I thought it would be better to specify the messages that we are verifying, otherwise the verify dialogService kind of loses its purpose if we use any() as the argument? I am not sure if verifying that dialogService gets invoked is enough in this test. If so, I can definitely switch them to any() arguments. @calixtus

I think it's okay to test the correct invocation. I think @calixtus and I thought verify only check with times the number of invovations.

calixtus · 2021-04-16T20:59:35Z

src/test/java/org/jabref/gui/edit/CopyMoreActionTest.java

+        String copiedTitles = String.join("\n", titles);
+        verify(clipBoardManager, times(1)).setContent(copiedTitles);
+        verify(dialogService, times(1)).notify(Localization.lang("Copied '%0' to clipboard.",
+                JabRefDialogService.shortenDialogMessage(copiedTitles)));


Test is becoming pretty complicated: in fact you are also testing the shortenDialogMessage-method here too...

Oh yes, I just realized this shortenDialogMessage method is being tested as well. However, as I mentioned in a comment above, the reason why I used specific localized argument was because the tests are meant to verify if dialogSerivce is showing the correct message under each condition. Maybe here I can just simply use any() as the mocked argument since it's too complicated.

I think it's okay.

Siedlerchr · 2021-04-18T17:54:15Z

@ningxie1991 Would be nice if you could address the hints related to the test. You can use mockito's any method without specifying the exact localization content etc
verify(dialogService, times(1)).notify(any());

* upstream/main: (354 commits) Fix ScienceDirect fetcher (#7684) Refactor NoBibTexFieldCheckerTest to increase mutation coverage (#7697) Update Gradle from 6.8.3 to 7.0 (#7619) Fixes Jabref#7305: the RFC fetcher is not compatible with the draftFix for issue 7305 (#7674) Refactoring existing unit tests (#7693) cover boundary cases & add more unit tests (#7694) Bump classgraph from 4.8.104 to 4.8.105 (#7688) Bump java-diff-utils from 4.9 to 4.10 (#7692) Fix arXiv fetcher tests (#7686) Make key for ScienceDirect configurable (#7683) migration of timestamp (#7671) Fix CCSB and DOAJ (#7426) [Bot] Update CSL styles (#7680) MS Office XML: Export month name (#7677) linkfix (#7678) readd fix (#7675) Fix threading cleanup in performSearch (#7672) add missing changelog delete bug fix (#7580) Add more unit tests to three gui classes (#7636) ... # Conflicts: # build.gradle

ningxie1991 and others added 10 commits April 15, 2021 10:02

Added two Junit tests using mocks for the gui

3dadc83

Add JUnit test with mocks for CopyMoreAction

9ac540f

organized imports

e631486

modified the tests

c06a2b6

Modified the tests

b860ca6

fix checkstyle issue and comment out the use of Globals

809b150

Extract Globals.prefs

d7dc08c

Update ExportToClipboardActionTest.java

027b968

Fix checkstyle issues

1119312

ningxie1991 changed the title ~~A3 nx~~ Add more unit tests to three gui classes Apr 16, 2021

ningxie1991 mentioned this pull request Apr 16, 2021

Add more unit tests to three gui classes #7635

Closed

5 tasks

Fix checkstyle issue

9af9b46

Siedlerchr requested a review from calixtus April 16, 2021 20:34

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

Siedlerchr approved these changes Apr 17, 2021

View reviewed changes

calixtus requested changes Apr 17, 2021

View reviewed changes

BShaq mentioned this pull request Apr 20, 2021

Add unit test to four test classes #7651

Merged

5 tasks

Siedlerchr merged commit 7aff889 into JabRef:main Apr 26, 2021

Siedlerchr mentioned this pull request Apr 26, 2021

Extract FileLinkPreferences and fileDirForDatabse JabRef/jabref-koppor#492

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more unit tests to three gui classes #7636

Add more unit tests to three gui classes #7636

ningxie1991 commented Apr 16, 2021

Siedlerchr left a comment

Siedlerchr commented Apr 17, 2021

calixtus left a comment

calixtus Apr 16, 2021

calixtus Apr 17, 2021

calixtus Apr 16, 2021

Siedlerchr Apr 17, 2021

calixtus Apr 17, 2021

calixtus Apr 17, 2021

Siedlerchr Apr 17, 2021

Siedlerchr Apr 17, 2021

calixtus Apr 17, 2021

calixtus Apr 17, 2021

calixtus Apr 18, 2021

calixtus Apr 16, 2021

calixtus Apr 16, 2021

Siedlerchr Apr 18, 2021

ningxie1991 Apr 18, 2021 •

edited

Loading

Siedlerchr Apr 20, 2021

calixtus Apr 16, 2021

ningxie1991 Apr 18, 2021

Siedlerchr Apr 20, 2021

Siedlerchr commented Apr 18, 2021

		FileLinkPreferences getFileLinkPreferences();

		void storeFileDirforDatabase(List<Path> dirs);

Add more unit tests to three gui classes #7636

Add more unit tests to three gui classes #7636

Conversation

ningxie1991 commented Apr 16, 2021

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr commented Apr 17, 2021

calixtus 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningxie1991 Apr 18, 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

Siedlerchr commented Apr 18, 2021

ningxie1991 Apr 18, 2021 •

edited

Loading