Add more unit tests #7638

Davfon · 2021-04-17T09:21:49Z

This pull request contributes to issue #6207, which is to add more unit tests to the project. The tests make use of mocks (stubbing) to simulate the behavior of some classes.

Tests added:

ImporterViewModelTest
BibStringDiffTest
PreambleDiffTest

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.

Siedlerchr · 2021-04-18T16:15:27Z

src/test/java/org/jabref/gui/importer/ImporterViewModelTest.java

+
+public class ImporterViewModelTest {
+
+    private final CustomImporter importer = mock(CustomImporter.class);


This whole test does not test any functionality. You are just testing java's getter and setter or property.
Don't just write tests for the sake of increasing testscoverage. Testing should concencrate on testing logic or implementaition

I can of course exclude this test, if you prefer so. I understand that testing logic and implementation is more valueable than increasing coverage. But higher coverage ultimately reaches the same goal. And according to the comment on issue #6207 the aim is to reach 100% code coverage.

I changed the text, because the knowledge that increasing coverage in an equal way is not as common as I assumed. I mean: If one class has 10% coverage, another has 90%, it is better to work on the class having 10% than trying to increase the coverage from 90% to 100% in the 90% class.

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

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

Siedlerchr · 2021-05-11T15:27:57Z

It would be really nice if you would finish this PR asap! @Davfon

Davfon · 2021-05-11T17:54:32Z

It would be really nice if you would finish this PR asap! @Davfon

Thanks for the feedback and sorry for the late response. I will try to do the changes by tomorrow evening.

Address the requested changes from #7638

Davfon · 2021-05-12T14:00:19Z

Should be all good now. Had to override the equals method. (And add hashCode() for checkstyle) Thanks again for the good inputs.

Siedlerchr · 2021-05-13T10:19:09Z

src/main/java/org/jabref/logic/bibtex/comparator/BibStringDiff.java

@@ -88,4 +88,18 @@ public BibtexString getOriginalString() {
    public BibtexString getNewString() {
        return newString;
    }
+
+    @Override
+    public boolean equals(Object other) {


Suggested change

public boolean equals(Object other) {

public boolean equals(Object other) {

if (this == other) {

return true;

}

if ((other == null) || (getClass() != other.getClass())) {

return false;

}

BibStringDiff that = (BibStringDiff) other;

return Objects.equals(newString, other.newString) && Objects.equals(originalString, that.originalString);

Do not use toString() beause the BIbTexString class has it's own equals methods which will then be called autoamtically internally.

And tip for the next time: Your IDE has an option to generate equals/hasCode methos

That's a good point, but since the BibtexString equals() method is strict and also compares the IDs of two BibtexStrings, it makes things a bit more complicated.

Is it intended, that two BibtexStrings that only differ in their ID are not considered equal? Because right now these would not be equal:

private final BibtexString bStr1 = new BibtexString("name", "content"); private final BibtexString bStr2 = new BibtexString("name", "content");

Since they get different IDs when they are created.

@koppor you have more knowledge of this BibtexStrings, can I have multiple ones with the same name?

@Davfon you might check the BibtexString dialog for some clues if there are additional restrictions

Strings are IMHO key/value pairs: https://docs.jabref.org/fields/strings.

I think, the Id fiels should not be treated when "equals". However, please, please have a look at the way how a BibEntry is compared and how Ids are treated. There should be a comment on that in the JavaDoc.

I think so too (that they are key/value pairs)

A BibEntry is compared by type, fields & commentsBeforeEntry. The Ids are not used for comparing. There was no JavaDoc for the equals method. I would suggest removing the Id comparison in the BibtexString.equals() method. (see last commit)

src/main/java/org/jabref/logic/bibtex/comparator/PreambleDiff.java

koppor

Some comments on tests

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

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

Siedlerchr · 2021-05-21T06:43:04Z

Thanks for the follow up fixes and as you can see you discovered an error in the equals implementation for the BibTexStrings

* upstream/main: Add more unit tests (#7638) Fix for issue 5804: Rewrite ACM fetcher (#7733)

Davfon added 2 commits April 17, 2021 11:03

Add more unit tests

fdc508f

Merge remote-tracking branch 'upstream/main' into a3-df

6ff6781

Siedlerchr requested changes Apr 18, 2021

View reviewed changes

Delete unnecessary test

1bf0a1d

koppor requested changes May 3, 2021

View reviewed changes

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

Davfon added 2 commits May 12, 2021 15:22

Override equals method for easier testing

f8b0ea0

Address the requested changes from #7638

Fix checkstyle

ef59b66

Siedlerchr requested changes May 13, 2021

View reviewed changes

Suggested changes & no Id comparison in BibtexString

46f6fb4

koppor requested changes May 18, 2021

View reviewed changes

src/test/java/org/jabref/logic/bibtex/comparator/PreambleDiffTest.java Outdated Show resolved Hide resolved

src/test/java/org/jabref/logic/bibtex/comparator/BibStringDiffTest.java Outdated Show resolved Hide resolved

Compare optionals and compare lists

dcc4926

koppor approved these changes May 20, 2021

View reviewed changes

Siedlerchr approved these changes May 21, 2021

View reviewed changes

Siedlerchr merged commit 7196eb3 into JabRef:main May 21, 2021

Siedlerchr added a commit that referenced this pull request May 21, 2021

Merge remote-tracking branch 'upstream/main' into revert-7750-fixTest

3c5ad18

* upstream/main: Add more unit tests (#7638) Fix for issue 5804: Rewrite ACM fetcher (#7733)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more unit tests #7638

Add more unit tests #7638

Davfon commented Apr 17, 2021

Siedlerchr Apr 18, 2021

Davfon Apr 19, 2021

koppor May 3, 2021

Siedlerchr commented May 11, 2021

Davfon commented May 11, 2021

Davfon commented May 12, 2021

Siedlerchr May 13, 2021

Davfon May 13, 2021 •

edited

Loading

Siedlerchr May 13, 2021

koppor May 17, 2021

Davfon May 18, 2021

koppor left a comment

Siedlerchr commented May 21, 2021


		public class ImporterViewModelTest {

		private final CustomImporter importer = mock(CustomImporter.class);

-    public boolean equals(Object other) {
+    public boolean equals(Object other) {
+        if (this == other) {
+            return true;
+        }
+        if ((other == null) || (getClass() != other.getClass())) {
+            return false;
+        }
+           BibStringDiff that = (BibStringDiff) other;
+        return Objects.equals(newString, other.newString) && Objects.equals(originalString, that.originalString);

Add more unit tests #7638

Add more unit tests #7638

Conversation

Davfon commented Apr 17, 2021

Siedlerchr Apr 18, 2021

Choose a reason for hiding this comment

Davfon Apr 19, 2021

Choose a reason for hiding this comment

koppor May 3, 2021

Choose a reason for hiding this comment

Siedlerchr commented May 11, 2021

Davfon commented May 11, 2021

Davfon commented May 12, 2021

Siedlerchr May 13, 2021

Choose a reason for hiding this comment

Davfon May 13, 2021 • edited Loading

Choose a reason for hiding this comment

Siedlerchr May 13, 2021

Choose a reason for hiding this comment

koppor May 17, 2021

Choose a reason for hiding this comment

Davfon May 18, 2021

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

Siedlerchr commented May 21, 2021

Davfon May 13, 2021 •

edited

Loading