Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more unit tests #7638

Merged
merged 7 commits into from
May 21, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public class BibStringDiff {
private final BibtexString originalString;
private final BibtexString newString;

private BibStringDiff(BibtexString originalString, BibtexString newString) {
BibStringDiff(BibtexString originalString, BibtexString newString) {
this.originalString = originalString;
this.newString = newString;
}
Expand Down Expand Up @@ -88,4 +88,22 @@ public BibtexString getOriginalString() {
public BibtexString getNewString() {
return newString;
}

@Override
public boolean equals(Object other) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Davfon Davfon May 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if (this == other) {
return true;
}
if ((other == null) || (getClass() != other.getClass())) {
return false;
}

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

@Override
public int hashCode() {
return Objects.hash(originalString, newString);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.jabref.logic.bibtex.comparator;

import java.util.Objects;
import java.util.Optional;

import org.jabref.model.database.BibDatabaseContext;
Expand All @@ -9,7 +10,7 @@ public class PreambleDiff {
private final String originalPreamble;
private final String newPreamble;

private PreambleDiff(String originalPreamble, String newPreamble) {
PreambleDiff(String originalPreamble, String newPreamble) {
this.originalPreamble = originalPreamble;
this.newPreamble = newPreamble;
}
Expand All @@ -31,4 +32,22 @@ public String getNewPreamble() {
public String getOriginalPreamble() {
return originalPreamble;
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}
if ((other == null) || (getClass() != other.getClass())) {
return false;
}

PreambleDiff that = (PreambleDiff) other;
return Objects.equals(newPreamble, that.newPreamble) && Objects.equals(originalPreamble, that.originalPreamble);
}

@Override
public int hashCode() {
return Objects.hash(originalPreamble, newPreamble);
}
}
1 change: 0 additions & 1 deletion src/main/java/org/jabref/model/entry/BibtexString.java
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ public boolean equals(Object o) {
return (Objects.equals(hasChanged, that.hasChanged) &&
Objects.equals(name, that.name) &&
Objects.equals(content, that.content) &&
Objects.equals(id, that.id) &&
Objects.equals(type, that.type) &&
Objects.equals(parsedSerialization, that.parsedSerialization));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package org.jabref.logic.bibtex.comparator;

import java.util.List;

import org.jabref.model.database.BibDatabase;
import org.jabref.model.entry.BibtexString;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class BibStringDiffTest {

private final BibDatabase originalDataBase = mock(BibDatabase.class);
private final BibDatabase newDataBase = mock(BibDatabase.class);
private final BibStringDiff diff = new BibStringDiff(new BibtexString("name2", "content2"), new BibtexString("name2", "content3"));

@BeforeEach
void setUp() {
when(originalDataBase.hasNoStrings()).thenReturn(false);
when(newDataBase.hasNoStrings()).thenReturn(false);
when(originalDataBase.getStringValues()).thenReturn(List.of(new BibtexString("name", "content"), new BibtexString("name2", "content2")));
when(newDataBase.getStringValues()).thenReturn(List.of(new BibtexString("name", "content"), new BibtexString("name2", "content3")));
}

@Test
void compareTest() {
BibStringDiff result = BibStringDiff.compare(originalDataBase, newDataBase).get(0);
assertEquals(diff, result);
koppor marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package org.jabref.logic.bibtex.comparator;

import java.util.Optional;

import org.jabref.model.database.BibDatabase;
import org.jabref.model.database.BibDatabaseContext;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class PreambleDiffTest {

private final BibDatabaseContext originalDataBaseContext = mock(BibDatabaseContext.class);
private final BibDatabaseContext newDataBaseContext = mock(BibDatabaseContext.class);
private final BibDatabase originalDataBase = mock(BibDatabase.class);
private final BibDatabase newDataBase = mock(BibDatabase.class);

@BeforeEach
void setUp() {
when(originalDataBaseContext.getDatabase()).thenReturn(originalDataBase);
when(newDataBaseContext.getDatabase()).thenReturn(newDataBase);
}

@Test
void compareSamePreambleTest() {
when(originalDataBase.getPreamble()).thenReturn(Optional.of("preamble"));
when(newDataBase.getPreamble()).thenReturn(Optional.of("preamble"));

assertEquals(Optional.empty(), PreambleDiff.compare(originalDataBaseContext, newDataBaseContext));
}

@Test
void compareDifferentPreambleTest() {
when(originalDataBase.getPreamble()).thenReturn(Optional.of("preamble"));
when(newDataBase.getPreamble()).thenReturn(Optional.of("otherPreamble"));

PreambleDiff expected = new PreambleDiff("preamble", "otherPreamble");
PreambleDiff result = PreambleDiff.compare(originalDataBaseContext, newDataBaseContext).orElse(null);
assertEquals(expected, result);
koppor marked this conversation as resolved.
Show resolved Hide resolved
}
}