Add more unit tests to three gui classes #7636
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ | |
| import org.jabref.logic.journals.JournalAbbreviationRepository; | ||
| import org.jabref.logic.l10n.Language; | ||
| import org.jabref.logic.layout.LayoutFormatterPreferences; | ||
| import org.jabref.logic.layout.format.FileLinkPreferences; | ||
| import org.jabref.logic.layout.format.NameFormatterPreferences; | ||
| import org.jabref.logic.net.ProxyPreferences; | ||
| import org.jabref.logic.openoffice.OpenOfficePreferences; | ||
|
|
@@ -278,6 +279,10 @@ public interface PreferencesService { | |
|
|
||
| void storeShouldAutosave(boolean shouldAutosave); | ||
|
|
||
| FileLinkPreferences getFileLinkPreferences(); | ||
|
|
||
| void storeFileDirforDatabase(List<Path> dirs); | ||
|
|
||
|
Comment on lines
+282
to
+285
There was a problem hiding this comment. There was a problem hiding this comment. Yes. This setter is no fix for the hack, but a disguise. There was a problem hiding this comment. Keep it ugly, until it's fixed. 😏 There was a problem hiding this comment. 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. There was a problem hiding this comment. I would vote to create a follow up PR to fix this There was a problem hiding this comment. 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: There was a problem hiding this comment. But to say that again with most deep concern: The proposed change, to put these two methods into the preferencesService shall not happen! There was a problem hiding this comment. 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 😆 |
||
| //************************************************************************************************************* | ||
| // Import/Export preferences | ||
| //************************************************************************************************************* | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| package org.jabref.gui.edit; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| import javafx.collections.FXCollections; | ||
| import javafx.collections.ObservableList; | ||
|
|
||
| import org.jabref.gui.ClipBoardManager; | ||
| import org.jabref.gui.DialogService; | ||
| import org.jabref.gui.JabRefDialogService; | ||
| import org.jabref.gui.StateManager; | ||
| import org.jabref.gui.actions.StandardActions; | ||
| import org.jabref.logic.l10n.Localization; | ||
| import org.jabref.model.database.BibDatabase; | ||
| import org.jabref.model.database.BibDatabaseContext; | ||
| import org.jabref.model.entry.BibEntry; | ||
| import org.jabref.model.entry.field.StandardField; | ||
| import org.jabref.model.entry.types.StandardEntryType; | ||
| import org.jabref.preferences.PreferencesService; | ||
|
|
||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.mockito.Mockito.any; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.spy; | ||
| import static org.mockito.Mockito.times; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| public class CopyMoreActionTest { | ||
|
|
||
| private CopyMoreAction copyMoreAction; | ||
| private DialogService dialogService = spy(DialogService.class); | ||
| private ClipBoardManager clipBoardManager = mock(ClipBoardManager.class); | ||
| private PreferencesService preferencesService = mock(PreferencesService.class); | ||
| private StateManager stateManager = mock(StateManager.class); | ||
| private BibEntry entry; | ||
| private List<String> titles = new ArrayList<String>(); | ||
| private List<String> keys = new ArrayList<String>(); | ||
|
|
||
| @BeforeEach | ||
| public void setUp() { | ||
| String title = "A tale from the trenches"; | ||
| entry = new BibEntry(StandardEntryType.Misc) | ||
| .withField(StandardField.AUTHOR, "Souti Chattopadhyay and Nicholas Nelson and Audrey Au and Natalia Morales and Christopher Sanchez and Rahul Pandita and Anita Sarma") | ||
| .withField(StandardField.TITLE, title) | ||
| .withField(StandardField.YEAR, "2020") | ||
| .withField(StandardField.DOI, "10.1145/3377811.3380330") | ||
| .withField(StandardField.SUBTITLE, "cognitive biases and software development") | ||
| .withCitationKey("abc"); | ||
| titles.add(title); | ||
| keys.add("abc"); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExecuteOnFail() { | ||
| when(stateManager.getActiveDatabase()).thenReturn(Optional.empty()); | ||
| when(stateManager.getSelectedEntries()).thenReturn(FXCollections.emptyObservableList()); | ||
| copyMoreAction = new CopyMoreAction(StandardActions.COPY_TITLE, dialogService, stateManager, clipBoardManager, preferencesService); | ||
| copyMoreAction.execute(); | ||
|
|
||
| verify(clipBoardManager, times(0)).setContent(any(String.class)); | ||
| verify(dialogService, times(0)).notify(any(String.class)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExecuteCopyTitleWithNoTitle() { | ||
| BibEntry entryWithNoTitle = (BibEntry) entry.clone(); | ||
| entryWithNoTitle.clearField(StandardField.TITLE); | ||
| ObservableList<BibEntry> entriesWithNoTitles = FXCollections.observableArrayList(entryWithNoTitle); | ||
| BibDatabaseContext databaseContext = new BibDatabaseContext(new BibDatabase(entriesWithNoTitles)); | ||
|
|
||
| when(stateManager.getActiveDatabase()).thenReturn(Optional.ofNullable(databaseContext)); | ||
| when(stateManager.getSelectedEntries()).thenReturn(entriesWithNoTitles); | ||
| copyMoreAction = new CopyMoreAction(StandardActions.COPY_TITLE, dialogService, stateManager, clipBoardManager, preferencesService); | ||
| copyMoreAction.execute(); | ||
|
|
||
| verify(clipBoardManager, times(0)).setContent(any(String.class)); | ||
| verify(dialogService, times(1)).notify(Localization.lang("None of the selected entries have titles.")); | ||
|
There was a problem hiding this comment. I thought I remember that junit provides something like |
||
| } | ||
|
|
||
| @Test | ||
| public void testExecuteCopyTitleOnPartialSuccess() { | ||
| BibEntry entryWithNoTitle = (BibEntry) entry.clone(); | ||
| entryWithNoTitle.clearField(StandardField.TITLE); | ||
| ObservableList<BibEntry> mixedEntries = FXCollections.observableArrayList(entryWithNoTitle, entry); | ||
| BibDatabaseContext databaseContext = new BibDatabaseContext(new BibDatabase(mixedEntries)); | ||
|
|
||
| when(stateManager.getActiveDatabase()).thenReturn(Optional.ofNullable(databaseContext)); | ||
| when(stateManager.getSelectedEntries()).thenReturn(mixedEntries); | ||
| copyMoreAction = new CopyMoreAction(StandardActions.COPY_TITLE, dialogService, stateManager, clipBoardManager, preferencesService); | ||
| copyMoreAction.execute(); | ||
|
|
||
| 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.", | ||
|
There was a problem hiding this comment. Same here: can the localized argument be exchanged with something generic? There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. |
||
| Integer.toString(mixedEntries.size() - titles.size()), Integer.toString(mixedEntries.size()))); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExecuteCopyTitleOnSuccess() { | ||
| ObservableList<BibEntry> entriesWithTitles = FXCollections.observableArrayList(entry); | ||
| BibDatabaseContext databaseContext = new BibDatabaseContext(new BibDatabase(entriesWithTitles)); | ||
|
|
||
| when(stateManager.getActiveDatabase()).thenReturn(Optional.ofNullable(databaseContext)); | ||
| when(stateManager.getSelectedEntries()).thenReturn(entriesWithTitles); | ||
| copyMoreAction = new CopyMoreAction(StandardActions.COPY_TITLE, dialogService, stateManager, clipBoardManager, preferencesService); | ||
| copyMoreAction.execute(); | ||
|
|
||
| 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))); | ||
|
There was a problem hiding this comment. Test is becoming pretty complicated: in fact you are also testing the shortenDialogMessage-method here too... There was a problem hiding this comment. 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. |
||
| } | ||
|
|
||
| @Test | ||
| public void testExecuteCopyKeyWithNoKey() { | ||
| BibEntry entryWithNoKey = (BibEntry) entry.clone(); | ||
| entryWithNoKey.clearCiteKey(); | ||
| ObservableList<BibEntry> entriesWithNoKeys = FXCollections.observableArrayList(entryWithNoKey); | ||
| BibDatabaseContext databaseContext = new BibDatabaseContext(new BibDatabase(entriesWithNoKeys)); | ||
|
|
||
| when(stateManager.getActiveDatabase()).thenReturn(Optional.ofNullable(databaseContext)); | ||
| when(stateManager.getSelectedEntries()).thenReturn(entriesWithNoKeys); | ||
| copyMoreAction = new CopyMoreAction(StandardActions.COPY_KEY, dialogService, stateManager, clipBoardManager, preferencesService); | ||
| copyMoreAction.execute(); | ||
|
|
||
| verify(clipBoardManager, times(0)).setContent(any(String.class)); | ||
| verify(dialogService, times(1)).notify(Localization.lang("None of the selected entries have citation keys.")); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExecuteCopyKeyOnPartialSuccess() { | ||
| BibEntry entryWithNoKey = (BibEntry) entry.clone(); | ||
| entryWithNoKey.clearCiteKey(); | ||
| ObservableList<BibEntry> mixedEntries = FXCollections.observableArrayList(entryWithNoKey, entry); | ||
| BibDatabaseContext databaseContext = new BibDatabaseContext(new BibDatabase(mixedEntries)); | ||
|
|
||
| when(stateManager.getActiveDatabase()).thenReturn(Optional.ofNullable(databaseContext)); | ||
| when(stateManager.getSelectedEntries()).thenReturn(mixedEntries); | ||
| copyMoreAction = new CopyMoreAction(StandardActions.COPY_KEY, dialogService, stateManager, clipBoardManager, preferencesService); | ||
| copyMoreAction.execute(); | ||
|
|
||
| String copiedKeys = String.join("\n", keys); | ||
| verify(clipBoardManager, times(1)).setContent(copiedKeys); | ||
| verify(dialogService, times(1)).notify(Localization.lang("Warning: %0 out of %1 entries have undefined citation key.", | ||
| Integer.toString(mixedEntries.size() - titles.size()), Integer.toString(mixedEntries.size()))); | ||
| } | ||
|
|
||
| @Test | ||
| public void testExecuteCopyKeyOnSuccess() { | ||
| ObservableList<BibEntry> entriesWithKeys = FXCollections.observableArrayList(entry); | ||
| BibDatabaseContext databaseContext = new BibDatabaseContext(new BibDatabase(entriesWithKeys)); | ||
|
|
||
| when(stateManager.getActiveDatabase()).thenReturn(Optional.ofNullable(databaseContext)); | ||
| when(stateManager.getSelectedEntries()).thenReturn(entriesWithKeys); | ||
| copyMoreAction = new CopyMoreAction(StandardActions.COPY_KEY, dialogService, stateManager, clipBoardManager, preferencesService); | ||
| copyMoreAction.execute(); | ||
|
|
||
| String copiedKeys = String.join("\n", keys); | ||
| verify(clipBoardManager, times(1)).setContent(copiedKeys); | ||
| verify(dialogService, times(1)).notify(Localization.lang("Copied '%0' to clipboard.", | ||
| JabRefDialogService.shortenDialogMessage(copiedKeys))); | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getFileLinkPreferencesneeds to be removed in the future as well as the variablefileDirForDatabase, 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. FurthermorestoreFileDirforDatabasedoes not persistently store a variable, therefor the naming scheme get/store should not be used here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this variable is better located in the stateManager? @tobiasdiez