ChangeBundle: Allow timestamp slop instead of rounding
ChangeReubilder will batch events together up to a maximum window size of 3 seconds (MAX_WINDOW_MS). This means that entities from the end of the window might have a timestamp in NoteDb up to 3 seconds earlier than the original timestamp in ReviewDb. This can't be accounted for by simply rounding the timestamp, as the amount we would have to subtract actually depends on *other* entities in the database. Instead, don't emit diffs if two timestamps are within the constant defined in ChangeRebuilder. This requires a different implementation of diffing ChangeMessages, since there is no longer a concept of normalizing timestamps. Instead we just ort the input list and try to pair them up. This may result in difficult to read diffs if somehow the relative timestamps got swapped, but we don't think this is a very common class of bugs. Change-Id: Ie226123739b91a3c4d47b50115d1f033221ae28f
This commit is contained in:
Dave Borowitz
parent
5ae24f5d23
commit
1693e1b8ef
@ -124,4 +124,15 @@ public final class ChangeMessage {
|
||||
public void setPatchSetId(PatchSet.Id id) {
|
||||
patchset = id;
|
||||
}
|
||||
|
||||
@Override
|
||||
public String toString() {
|
||||
return "ChangeMessage{"
|
||||
+ "key=" + key
|
||||
+ ", author=" + author
|
||||
+ ", writtenOn=" + writtenOn
|
||||
+ ", patchset=" + patchset
|
||||
+ ", message=[" + message
|
||||
+ "]}";
|
||||
}
|
||||
}
|
||||
|
||||
@ -21,19 +21,13 @@ import static com.google.gerrit.common.TimeUtil.roundToSecond;
|
||||
import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB;
|
||||
import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB;
|
||||
|
||||
import com.google.auto.value.AutoValue;
|
||||
import com.google.common.annotations.VisibleForTesting;
|
||||
import com.google.common.base.Joiner;
|
||||
import com.google.common.collect.ComparisonChain;
|
||||
import com.google.common.collect.ImmutableList;
|
||||
import com.google.common.collect.ImmutableMap;
|
||||
import com.google.common.collect.Iterables;
|
||||
import com.google.common.collect.Multiset;
|
||||
import com.google.common.collect.Ordering;
|
||||
import com.google.common.collect.Sets;
|
||||
import com.google.common.collect.TreeMultiset;
|
||||
import com.google.gerrit.common.Nullable;
|
||||
import com.google.gerrit.common.TimeUtil;
|
||||
import com.google.gerrit.reviewdb.client.Account;
|
||||
import com.google.gerrit.reviewdb.client.Change;
|
||||
import com.google.gerrit.reviewdb.client.ChangeMessage;
|
||||
import com.google.gerrit.reviewdb.client.Patch;
|
||||
@ -43,7 +37,6 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
|
||||
import com.google.gerrit.reviewdb.server.ReviewDb;
|
||||
import com.google.gerrit.server.PatchLineCommentsUtil;
|
||||
import com.google.gwtorm.client.Column;
|
||||
import com.google.gwtorm.client.IntKey;
|
||||
import com.google.gwtorm.server.OrmException;
|
||||
|
||||
import java.lang.reflect.Field;
|
||||
@ -205,7 +198,7 @@ public class ChangeBundle {
|
||||
}
|
||||
|
||||
private final Change change;
|
||||
private final ImmutableMap<ChangeMessage.Key, ChangeMessage> changeMessages;
|
||||
private final ImmutableList<ChangeMessage> changeMessages;
|
||||
private final ImmutableMap<PatchSet.Id, PatchSet> patchSets;
|
||||
private final ImmutableMap<PatchSetApproval.Key, PatchSetApproval>
|
||||
patchSetApprovals;
|
||||
@ -222,7 +215,8 @@ public class ChangeBundle {
|
||||
Iterable<PatchLineComment> patchLineComments,
|
||||
Source source) {
|
||||
this.change = checkNotNull(change);
|
||||
this.changeMessages = ImmutableMap.copyOf(changeMessageMap(changeMessages));
|
||||
this.changeMessages =
|
||||
ChangeNotes.MESSAGE_BY_TIME.immutableSortedCopy(changeMessages);
|
||||
this.patchSets = ImmutableMap.copyOf(patchSetMap(patchSets));
|
||||
this.patchSetApprovals =
|
||||
ImmutableMap.copyOf(patchSetApprovalMap(patchSetApprovals));
|
||||
@ -230,8 +224,8 @@ public class ChangeBundle {
|
||||
ImmutableMap.copyOf(patchLineCommentMap(patchLineComments));
|
||||
this.source = checkNotNull(source);
|
||||
|
||||
for (ChangeMessage.Key k : this.changeMessages.keySet()) {
|
||||
checkArgument(k.getParentKey().equals(change.getId()));
|
||||
for (ChangeMessage m : this.changeMessages) {
|
||||
checkArgument(m.getKey().getParentKey().equals(change.getId()));
|
||||
}
|
||||
for (PatchSet.Id id : this.patchSets.keySet()) {
|
||||
checkArgument(id.getParentKey().equals(change.getId()));
|
||||
@ -271,8 +265,10 @@ public class ChangeBundle {
|
||||
ChangeBundle bundleA, ChangeBundle bundleB) {
|
||||
if (bundleA.source == REVIEW_DB && bundleB.source == REVIEW_DB) {
|
||||
// Both came from ReviewDb: check all fields exactly.
|
||||
Map<ChangeMessage.Key, ChangeMessage> as = bundleA.changeMessages;
|
||||
Map<ChangeMessage.Key, ChangeMessage> bs = bundleB.changeMessages;
|
||||
Map<ChangeMessage.Key, ChangeMessage> as =
|
||||
changeMessageMap(bundleA.changeMessages);
|
||||
Map<ChangeMessage.Key, ChangeMessage> bs =
|
||||
changeMessageMap(bundleB.changeMessages);
|
||||
|
||||
for (ChangeMessage.Key k : diffKeySets(diffs, as, bs)) {
|
||||
ChangeMessage a = as.get(k);
|
||||
@ -280,29 +276,29 @@ public class ChangeBundle {
|
||||
String desc = describe(k);
|
||||
diffColumns(diffs, ChangeMessage.class, desc, bundleA, a, bundleB, b);
|
||||
}
|
||||
} else {
|
||||
// At least one is from NoteDb, so we need to normalize UUIDs and
|
||||
// timestamps for both.
|
||||
Multiset<NormalizedChangeMessage> as = bundleA.normalizeChangeMessages();
|
||||
Multiset<NormalizedChangeMessage> bs = bundleB.normalizeChangeMessages();
|
||||
Set<NormalizedChangeMessage> union = new TreeSet<>();
|
||||
for (NormalizedChangeMessage m
|
||||
: Iterables.concat(as.elementSet(), bs.elementSet())) {
|
||||
union.add(m);
|
||||
}
|
||||
for (NormalizedChangeMessage m : union) {
|
||||
int ac = as.count(m);
|
||||
int bc = bs.count(m);
|
||||
if (ac != bc) {
|
||||
diffs.add("ChangeMessage present "
|
||||
+ times(ac) + " in A but " + times(bc) + " in B: " + m);
|
||||
}
|
||||
}
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
private static String times(int n) {
|
||||
return n + " time" + (n != 1 ? "s" : "");
|
||||
// At least one is from NoteDb, so we need to ignore UUIDs for both, and
|
||||
// allow timestamp slop if the sources differ.
|
||||
Change.Id id = bundleA.getChange().getId();
|
||||
checkArgument(id.equals(bundleB.getChange().getId()));
|
||||
List<ChangeMessage> as = bundleA.changeMessages;
|
||||
List<ChangeMessage> bs = bundleB.changeMessages;
|
||||
if (as.size() != bs.size()) {
|
||||
Joiner j = Joiner.on("\n");
|
||||
diffs.add("Differing numbers of ChangeMessages for Change.Id " + id
|
||||
+ ":\n" + j.join(as) + "\n--- vs. ---\n" + j.join(bs));
|
||||
return;
|
||||
}
|
||||
|
||||
for (int i = 0; i < as.size(); i++) {
|
||||
ChangeMessage a = as.get(i);
|
||||
ChangeMessage b = bs.get(i);
|
||||
String desc = "ChangeMessage on " + id + " at index " + i;
|
||||
diffColumnsExcluding(diffs, ChangeMessage.class, desc, bundleA, a,
|
||||
bundleB, b, "key");
|
||||
}
|
||||
}
|
||||
|
||||
private static void diffPatchSets(List<String> diffs, ChangeBundle bundleA,
|
||||
@ -362,10 +358,19 @@ public class ChangeBundle {
|
||||
|
||||
private static <T> void diffColumns(List<String> diffs, Class<T> clazz,
|
||||
String desc, ChangeBundle bundleA, T a, ChangeBundle bundleB, T b) {
|
||||
diffColumnsExcluding(diffs, clazz, desc, bundleA, a, bundleB, b);
|
||||
}
|
||||
|
||||
private static <T> void diffColumnsExcluding(List<String> diffs,
|
||||
Class<T> clazz, String desc, ChangeBundle bundleA, T a,
|
||||
ChangeBundle bundleB, T b, String... exclude) {
|
||||
Set<String> toExclude = Sets.newLinkedHashSet(Arrays.asList(exclude));
|
||||
for (Field f : clazz.getDeclaredFields()) {
|
||||
Column col = f.getAnnotation(Column.class);
|
||||
if (col == null) {
|
||||
continue;
|
||||
} else if (toExclude.remove(f.getName())) {
|
||||
continue;
|
||||
}
|
||||
f.setAccessible(true);
|
||||
try {
|
||||
@ -378,6 +383,9 @@ public class ChangeBundle {
|
||||
throw new IllegalArgumentException(e);
|
||||
}
|
||||
}
|
||||
checkArgument(toExclude.isEmpty(),
|
||||
"requested columns to exclude not present in %s: %s",
|
||||
clazz.getSimpleName(), toExclude);
|
||||
}
|
||||
|
||||
private static void diffTimestamps(List<String> diffs, String desc,
|
||||
@ -397,18 +405,30 @@ public class ChangeBundle {
|
||||
| SecurityException e) {
|
||||
throw new IllegalArgumentException(e);
|
||||
}
|
||||
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
|
||||
ta = roundToSecond(ta);
|
||||
checkArgument(tb.equals(roundToSecond(tb)),
|
||||
"%s from NoteDb has non-rounded %s timestamp: %s",
|
||||
desc, field, tb);
|
||||
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
|
||||
tb = roundToSecond(tb);
|
||||
checkArgument(ta.equals(roundToSecond(ta)),
|
||||
"%s from NoteDb has non-rounded %s timestamp: %s",
|
||||
desc, field, ta);
|
||||
if (bundleA.source == bundleB.source || ta == null || tb == null) {
|
||||
diffValues(diffs, desc, ta, tb, field);
|
||||
} else if (bundleA.source == NOTE_DB) {
|
||||
diffTimestamps(diffs, desc, ta, tb, field);
|
||||
} else {
|
||||
diffTimestamps(diffs, desc, tb, ta, field);
|
||||
}
|
||||
}
|
||||
|
||||
private static void diffTimestamps(List<String> diffs, String desc,
|
||||
Timestamp tsFromNoteDb, Timestamp tsFromReviewDb, String field) {
|
||||
// Because ChangeRebuilder may batch events together that are several
|
||||
// seconds apart, the timestamp in NoteDb may actually be several seconds
|
||||
// *earlier* than the timestamp in ReviewDb that it was converted from.
|
||||
checkArgument(tsFromNoteDb.equals(roundToSecond(tsFromNoteDb)),
|
||||
"%s from NoteDb has non-rounded %s timestamp: %s",
|
||||
desc, field, tsFromNoteDb);
|
||||
long delta = tsFromReviewDb.getTime() - tsFromNoteDb.getTime();
|
||||
long max = ChangeRebuilder.MAX_WINDOW_MS;
|
||||
if (delta < 0 || delta > max) {
|
||||
diffs.add(
|
||||
field + " differs for " + desc + " in NoteDb vs. ReviewDb:"
|
||||
+ " {" + tsFromNoteDb + "} != {" + tsFromReviewDb + "}");
|
||||
}
|
||||
diffValues(diffs, desc, ta, tb, field);
|
||||
}
|
||||
|
||||
private static void diffValues(List<String> diffs, String desc, Object va,
|
||||
@ -431,51 +451,6 @@ public class ChangeBundle {
|
||||
return clazz.getEnclosingClass().getSimpleName() + "." + name;
|
||||
}
|
||||
|
||||
@AutoValue
|
||||
static abstract class NormalizedChangeMessage
|
||||
implements Comparable<NormalizedChangeMessage> {
|
||||
private static final Ordering<Comparable<?>> NULLS_FIRST =
|
||||
Ordering.natural().nullsFirst();
|
||||
|
||||
private static NormalizedChangeMessage create(ChangeMessage msg) {
|
||||
return new AutoValue_ChangeBundle_NormalizedChangeMessage(
|
||||
msg.getKey().getParentKey(),
|
||||
msg.getAuthor(),
|
||||
TimeUtil.roundToSecond(msg.getWrittenOn()),
|
||||
msg.getMessage(),
|
||||
msg.getPatchSetId());
|
||||
}
|
||||
|
||||
private static Integer intKey(IntKey<?> k) {
|
||||
return k != null ? k.get() : null;
|
||||
}
|
||||
|
||||
abstract Change.Id changeId();
|
||||
@Nullable abstract Account.Id author();
|
||||
abstract Timestamp writtenOn();
|
||||
@Nullable abstract String message();
|
||||
@Nullable abstract PatchSet.Id patchset();
|
||||
|
||||
@Override
|
||||
public int compareTo(NormalizedChangeMessage o) {
|
||||
return ComparisonChain.start()
|
||||
.compare(changeId().get(), o.changeId().get())
|
||||
.compare(intKey(patchset()), intKey(o.patchset()), NULLS_FIRST)
|
||||
.compare(writtenOn(), o.writtenOn())
|
||||
.compare(intKey(author()), intKey(o.author()), NULLS_FIRST)
|
||||
.compare(message(), o.message(), NULLS_FIRST)
|
||||
.result();
|
||||
}
|
||||
}
|
||||
|
||||
private Multiset<NormalizedChangeMessage> normalizeChangeMessages() {
|
||||
Multiset<NormalizedChangeMessage> normalized = TreeMultiset.create();
|
||||
for (ChangeMessage msg : changeMessages.values()) {
|
||||
normalized.add(NormalizedChangeMessage.create(msg));
|
||||
}
|
||||
return normalized;
|
||||
}
|
||||
|
||||
@Override
|
||||
public String toString() {
|
||||
return getClass().getSimpleName() + "{id=" + change.getId()
|
||||
|
||||
@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkState;
|
||||
import static com.google.gerrit.server.PatchLineCommentsUtil.setCommentRevId;
|
||||
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_HASHTAGS;
|
||||
import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET;
|
||||
import static java.util.concurrent.TimeUnit.SECONDS;
|
||||
|
||||
import com.google.common.base.MoreObjects;
|
||||
import com.google.common.base.Splitter;
|
||||
@ -73,13 +74,26 @@ import java.util.List;
|
||||
import java.util.Objects;
|
||||
import java.util.Set;
|
||||
import java.util.concurrent.Callable;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
import java.util.regex.Matcher;
|
||||
import java.util.regex.Pattern;
|
||||
|
||||
public class ChangeRebuilder {
|
||||
private static final long TS_WINDOW_MS =
|
||||
TimeUnit.MILLISECONDS.convert(1, TimeUnit.SECONDS);
|
||||
/**
|
||||
* The maximum amount of time between the ReviewDb timestamp of the first and
|
||||
* last events batched together into a single NoteDb update.
|
||||
* <p>
|
||||
* Used to account for the fact that different records with their own
|
||||
* timestamps (e.g. {@link PatchSetApproval} and {@link ChangeMessage})
|
||||
* historically didn't necessarily use the same timestamp, and tended to call
|
||||
* {@code System.currentTimeMillis()} independently.
|
||||
*/
|
||||
static final long MAX_WINDOW_MS = SECONDS.toMillis(3);
|
||||
|
||||
/**
|
||||
* The maximum amount of time between two consecutive events to consider them
|
||||
* to be in the same batch.
|
||||
*/
|
||||
private static final long MAX_DELTA_MS = SECONDS.toMillis(1);
|
||||
|
||||
private final SchemaFactory<ReviewDb> schemaFactory;
|
||||
private final GitRepositoryManager repoManager;
|
||||
@ -350,7 +364,7 @@ public class ChangeRebuilder {
|
||||
checkState(Objects.equals(update.getPatchSetId(), psId),
|
||||
"cannot apply event for %s to update for %s",
|
||||
update.getPatchSetId(), psId);
|
||||
checkState(when.getTime() - update.getWhen().getTime() <= TS_WINDOW_MS,
|
||||
checkState(when.getTime() - update.getWhen().getTime() <= MAX_WINDOW_MS,
|
||||
"event at %s outside update window starting at %s",
|
||||
when, update.getWhen());
|
||||
checkState(Objects.equals(update.getUser().getAccountId(), who),
|
||||
@ -379,9 +393,6 @@ public class ChangeRebuilder {
|
||||
private class EventList<E extends Event> extends ArrayList<E> {
|
||||
private static final long serialVersionUID = 1L;
|
||||
|
||||
private static final long MAX_DELTA_MS = 1000;
|
||||
private static final long MAX_WINDOW_MS = 5000;
|
||||
|
||||
private E getLast() {
|
||||
return get(size() - 1);
|
||||
}
|
||||
|
||||
@ -77,7 +77,9 @@ public class ChangeBundleTest {
|
||||
systemTimeZoneProperty = System.setProperty("user.timezone", tz);
|
||||
systemTimeZone = TimeZone.getDefault();
|
||||
TimeZone.setDefault(TimeZone.getTimeZone(tz));
|
||||
TestTimeUtil.resetWithClockStep(1, SECONDS);
|
||||
long maxMs = ChangeRebuilder.MAX_WINDOW_MS;
|
||||
assertThat(maxMs).isGreaterThan(1000L);
|
||||
TestTimeUtil.resetWithClockStep(maxMs * 2, MILLISECONDS);
|
||||
project = new Project.NameKey("project");
|
||||
accountId = new Account.Id(100);
|
||||
}
|
||||
@ -89,8 +91,14 @@ public class ChangeBundleTest {
|
||||
TimeZone.setDefault(systemTimeZone);
|
||||
}
|
||||
|
||||
private void subSecondResolution() {
|
||||
TestTimeUtil.setClockStep(100, MILLISECONDS);
|
||||
private void superWindowResolution() {
|
||||
TestTimeUtil.setClockStep(
|
||||
ChangeRebuilder.MAX_WINDOW_MS * 2, MILLISECONDS);
|
||||
TimeUtil.nowTs();
|
||||
}
|
||||
|
||||
private void subWindowResolution() {
|
||||
TestTimeUtil.setClockStep(1, SECONDS);
|
||||
TimeUtil.nowTs();
|
||||
}
|
||||
|
||||
@ -108,9 +116,9 @@ public class ChangeBundleTest {
|
||||
assertDiffs(b1, b2,
|
||||
"changeId differs for Changes: {" + id1 + "} != {" + id2 + "}",
|
||||
"createdOn differs for Changes:"
|
||||
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:01.0}",
|
||||
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:06.0}",
|
||||
"lastUpdatedOn differs for Changes:"
|
||||
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:01.0}");
|
||||
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:06.0}");
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -131,10 +139,10 @@ public class ChangeBundleTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
public void diffChangesMixedSourcesRoundsTimestamp() throws Exception {
|
||||
public void diffChangesMixedSourcesAllowsSlop() throws Exception {
|
||||
subWindowResolution();
|
||||
Change c1 = TestChanges.newChange(
|
||||
new Project.NameKey("project"), new Account.Id(100));
|
||||
subSecondResolution();
|
||||
Change c2 = clone(c1);
|
||||
c2.setCreatedOn(TimeUtil.nowTs());
|
||||
c2.setLastUpdatedOn(TimeUtil.nowTs());
|
||||
@ -146,20 +154,31 @@ public class ChangeBundleTest {
|
||||
approvals(), comments(), REVIEW_DB);
|
||||
assertDiffs(b1, b2,
|
||||
"createdOn differs for Change.Id " + c1.getId() + ":"
|
||||
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:01.1}",
|
||||
+ " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:02.0}",
|
||||
"lastUpdatedOn differs for Change.Id " + c1.getId() + ":"
|
||||
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:01.2}");
|
||||
+ " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:03.0}");
|
||||
|
||||
// One NoteDb, timestamp is rounded.
|
||||
// One NoteDb, slop is allowed.
|
||||
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
|
||||
comments(), NOTE_DB);
|
||||
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
|
||||
comments(), REVIEW_DB);
|
||||
assertDiffs(b1, b2,
|
||||
"createdOn differs for Change.Id " + c1.getId() + ":"
|
||||
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:01.0}",
|
||||
"lastUpdatedOn differs for Change.Id " + c1.getId() + ":"
|
||||
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:01.0}");
|
||||
assertNoDiffs(b1, b2);
|
||||
assertNoDiffs(b2, b1);
|
||||
|
||||
// But not too much slop.
|
||||
superWindowResolution();
|
||||
Change c3 = clone(c1);
|
||||
c3.setLastUpdatedOn(TimeUtil.nowTs());
|
||||
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
|
||||
comments(), NOTE_DB);
|
||||
ChangeBundle b3 = new ChangeBundle(c3, messages(), patchSets(), approvals(),
|
||||
comments(), REVIEW_DB);
|
||||
String msg = "lastUpdatedOn differs for Change.Id " + c1.getId()
|
||||
+ " in NoteDb vs. ReviewDb:"
|
||||
+ " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:10.0}";
|
||||
assertDiffs(b1, b3, msg);
|
||||
assertDiffs(b3, b1, msg);
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -231,6 +250,46 @@ public class ChangeBundleTest {
|
||||
assertNoDiffs(b1, b2);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void diffChangeMessagesWithDifferentCounts() throws Exception {
|
||||
Change c = TestChanges.newChange(project, accountId);
|
||||
int id = c.getId().get();
|
||||
ChangeMessage cm1 = new ChangeMessage(
|
||||
new ChangeMessage.Key(c.getId(), "uuid1"),
|
||||
accountId, TimeUtil.nowTs(), c.currentPatchSetId());
|
||||
cm1.setMessage("message 1");
|
||||
ChangeMessage cm2 = new ChangeMessage(
|
||||
new ChangeMessage.Key(c.getId(), "uuid2"),
|
||||
accountId, TimeUtil.nowTs(), c.currentPatchSetId());
|
||||
cm1.setMessage("message 2");
|
||||
|
||||
// Both ReviewDb: Uses same keySet diff as other types.
|
||||
ChangeBundle b1 = new ChangeBundle(c, messages(cm1, cm2), patchSets(),
|
||||
approvals(), comments(), REVIEW_DB);
|
||||
ChangeBundle b2 = new ChangeBundle(c, messages(cm1), patchSets(),
|
||||
approvals(), comments(), REVIEW_DB);
|
||||
assertDiffs(b1, b2,
|
||||
"ChangeMessage.Key sets differ: [" + id
|
||||
+ ",uuid2] only in A; [] only in B");
|
||||
|
||||
// One NoteDb: UUIDs in keys can't be used for comparison, just diff counts.
|
||||
b1 = new ChangeBundle(c, messages(cm1, cm2), patchSets(), approvals(),
|
||||
comments(), REVIEW_DB);
|
||||
b2 = new ChangeBundle(c, messages(cm1), patchSets(), approvals(),
|
||||
comments(), NOTE_DB);
|
||||
assertDiffs(b1, b2,
|
||||
"Differing numbers of ChangeMessages for Change.Id " + id + ":\n"
|
||||
+ "ChangeMessage{key=" + id + ",uuid1, author=100,"
|
||||
+ " writtenOn=2009-09-30 17:00:06.0, patchset=" + id + ",1,"
|
||||
+ " message=[message 2]}\n"
|
||||
+ "ChangeMessage{key=" + id + ",uuid2, author=100,"
|
||||
+ " writtenOn=2009-09-30 17:00:12.0, patchset=" + id + ",1,"
|
||||
+ " message=[null]}\n"
|
||||
+ "--- vs. ---\n"
|
||||
+ "ChangeMessage{key=" + id + ",uuid1, author=100,"
|
||||
+ " writtenOn=2009-09-30 17:00:06.0, patchset=" + id + ",1,"
|
||||
+ " message=[message 2]}");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void diffChangeMessagesMixedSourcesWithDifferences() throws Exception {
|
||||
@ -250,20 +309,14 @@ public class ChangeBundleTest {
|
||||
ChangeBundle b2 = new ChangeBundle(c, messages(cm2, cm3), patchSets(),
|
||||
approvals(), comments(), NOTE_DB);
|
||||
assertDiffs(b1, b2,
|
||||
"ChangeMessage present 2 times in A but 1 time in B:"
|
||||
+ " NormalizedChangeMessage{changeId=" + id + ", author=100,"
|
||||
+ " writtenOn=2009-09-30 17:00:01.0, message=message 1,"
|
||||
+ " patchset=" + id + ",1}",
|
||||
"ChangeMessage present 0 times in A but 1 time in B:"
|
||||
+ " NormalizedChangeMessage{changeId=" + id + ", author=100,"
|
||||
+ " writtenOn=2009-09-30 17:00:01.0, message=message 2,"
|
||||
+ " patchset=" + id + ",1}");
|
||||
"message differs for ChangeMessage on " + id + " at index 0:"
|
||||
+ " {message 1} != {message 2}");
|
||||
}
|
||||
|
||||
@Test
|
||||
public void diffChangeMessagesMixedSourcesRoundsTimestamp() throws Exception {
|
||||
public void diffChangeMessagesMixedSourcesAllowsSlop() throws Exception {
|
||||
subWindowResolution();
|
||||
Change c = TestChanges.newChange(project, accountId);
|
||||
subSecondResolution();
|
||||
ChangeMessage cm1 = new ChangeMessage(
|
||||
new ChangeMessage.Key(c.getId(), "uuid1"),
|
||||
accountId, TimeUtil.nowTs(), c.currentPatchSetId());
|
||||
@ -277,14 +330,29 @@ public class ChangeBundleTest {
|
||||
approvals(), comments(), REVIEW_DB);
|
||||
assertDiffs(b1, b2,
|
||||
"writtenOn differs for ChangeMessage.Key " + c.getId() + ",uuid1:"
|
||||
+ " {2009-09-30 17:00:01.1} != {2009-09-30 17:00:01.2}");
|
||||
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:03.0}");
|
||||
|
||||
// One NoteDb, timestamp is rounded.
|
||||
// One NoteDb, slop is allowed.
|
||||
b1 = new ChangeBundle(c, messages(cm1), patchSets(), approvals(),
|
||||
comments(), NOTE_DB);
|
||||
b2 = new ChangeBundle(c, messages(cm2), patchSets(), approvals(),
|
||||
comments(), REVIEW_DB);
|
||||
assertNoDiffs(b1, b2);
|
||||
assertNoDiffs(b2, b1);
|
||||
|
||||
// But not too much slop.
|
||||
superWindowResolution();
|
||||
ChangeMessage cm3 = clone(cm1);
|
||||
cm3.setWrittenOn(TimeUtil.nowTs());
|
||||
b1 = new ChangeBundle(c, messages(cm1), patchSets(), approvals(),
|
||||
comments(), NOTE_DB);
|
||||
ChangeBundle b3 = new ChangeBundle(c, messages(cm3), patchSets(),
|
||||
approvals(), comments(), REVIEW_DB);
|
||||
String msg = "writtenOn differs for ChangeMessage on " + c.getId() +
|
||||
" at index 0 in NoteDb vs. ReviewDb:"
|
||||
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:10.0}";
|
||||
assertDiffs(b1, b3, msg);
|
||||
assertDiffs(b3, b1, msg);
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -334,9 +402,9 @@ public class ChangeBundleTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
public void diffPatchSetsMixedSourcesRoundsTimestamp() throws Exception {
|
||||
public void diffPatchSetsMixedSourcesAllowsSlop() throws Exception {
|
||||
subWindowResolution();
|
||||
Change c = TestChanges.newChange(project, accountId);
|
||||
subSecondResolution();
|
||||
PatchSet ps1 = new PatchSet(c.currentPatchSetId());
|
||||
ps1.setRevision(new RevId("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"));
|
||||
ps1.setUploader(accountId);
|
||||
@ -351,14 +419,28 @@ public class ChangeBundleTest {
|
||||
approvals(), comments(), REVIEW_DB);
|
||||
assertDiffs(b1, b2,
|
||||
"createdOn differs for PatchSet.Id " + c.getId() + ",1:"
|
||||
+ " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:01.2}");
|
||||
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:03.0}");
|
||||
|
||||
// One NoteDb, timestamp is rounded.
|
||||
// One NoteDb, slop is allowed.
|
||||
b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(),
|
||||
comments(), NOTE_DB);
|
||||
b2 = new ChangeBundle(c, messages(), patchSets(ps2), approvals(),
|
||||
comments(), REVIEW_DB);
|
||||
assertNoDiffs(b1, b2);
|
||||
|
||||
// But not too much slop.
|
||||
superWindowResolution();
|
||||
PatchSet ps3 = clone(ps1);
|
||||
ps3.setCreatedOn(TimeUtil.nowTs());
|
||||
b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(),
|
||||
comments(), NOTE_DB);
|
||||
ChangeBundle b3 = new ChangeBundle(c, messages(), patchSets(ps3),
|
||||
approvals(), comments(), REVIEW_DB);
|
||||
String msg = "createdOn differs for PatchSet.Id " + c.getId()
|
||||
+ ",1 in NoteDb vs. ReviewDb:"
|
||||
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:10.0}";
|
||||
assertDiffs(b1, b3, msg);
|
||||
assertDiffs(b3, b1, msg);
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -410,10 +492,10 @@ public class ChangeBundleTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
public void diffPatchSetApprovalsMixedSourcesRoundsTimestamp()
|
||||
public void diffPatchSetApprovalsMixedSourcesAllowsSlop()
|
||||
throws Exception {
|
||||
Change c = TestChanges.newChange(project, accountId);
|
||||
subSecondResolution();
|
||||
subWindowResolution();
|
||||
PatchSetApproval a1 = new PatchSetApproval(
|
||||
new PatchSetApproval.Key(
|
||||
c.currentPatchSetId(), accountId, new LabelId("Code-Review")),
|
||||
@ -430,14 +512,28 @@ public class ChangeBundleTest {
|
||||
assertDiffs(b1, b2,
|
||||
"granted differs for PatchSetApproval.Key "
|
||||
+ c.getId() + "%2C1,100,Code-Review:"
|
||||
+ " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:01.2}");
|
||||
+ " {2009-09-30 17:00:07.0} != {2009-09-30 17:00:08.0}");
|
||||
|
||||
// One NoteDb, timestamp is rounded.
|
||||
// One NoteDb, slop is allowed.
|
||||
b1 = new ChangeBundle(c, messages(), patchSets(), approvals(a1),
|
||||
comments(), NOTE_DB);
|
||||
b2 = new ChangeBundle(c, messages(), patchSets(), approvals(a2),
|
||||
comments(), REVIEW_DB);
|
||||
assertNoDiffs(b1, b2);
|
||||
|
||||
// But not too much slop.
|
||||
superWindowResolution();
|
||||
PatchSetApproval a3 = clone(a1);
|
||||
a3.setGranted(TimeUtil.nowTs());
|
||||
b1 = new ChangeBundle(c, messages(), patchSets(), approvals(a1),
|
||||
comments(), NOTE_DB);
|
||||
ChangeBundle b3 = new ChangeBundle(c, messages(), patchSets(),
|
||||
approvals(a3), comments(), REVIEW_DB);
|
||||
String msg = "granted differs for PatchSetApproval.Key "
|
||||
+ c.getId() + "%2C1,100,Code-Review in NoteDb vs. ReviewDb:"
|
||||
+ " {2009-09-30 17:00:07.0} != {2009-09-30 17:00:15.0}";
|
||||
assertDiffs(b1, b3, msg);
|
||||
assertDiffs(b3, b1, msg);
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -486,10 +582,10 @@ public class ChangeBundleTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
public void diffPatchLineCommentsMixedSourcesRoundsTimestamp()
|
||||
public void diffPatchLineCommentsMixedSourcesAllowsSlop()
|
||||
throws Exception {
|
||||
subWindowResolution();
|
||||
Change c = TestChanges.newChange(project, accountId);
|
||||
subSecondResolution();
|
||||
PatchLineComment c1 = new PatchLineComment(
|
||||
new PatchLineComment.Key(
|
||||
new Patch.Key(c.currentPatchSetId(), "filename"), "uuid"),
|
||||
@ -505,15 +601,29 @@ public class ChangeBundleTest {
|
||||
assertDiffs(b1, b2,
|
||||
"writtenOn differs for PatchLineComment.Key "
|
||||
+ c.getId() + ",1,filename,uuid:"
|
||||
+ " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:01.2}");
|
||||
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:03.0}");
|
||||
|
||||
// One NoteDb, timestamp is rounded.
|
||||
// One NoteDb, slop is allowed.
|
||||
b1 = new ChangeBundle(c, messages(), patchSets(), approvals(),
|
||||
comments(c1), NOTE_DB);
|
||||
b2 = new ChangeBundle(c, messages(), patchSets(), approvals(),
|
||||
comments(c2), REVIEW_DB);
|
||||
assertNoDiffs(b1, b2);
|
||||
}
|
||||
|
||||
// But not too much slop.
|
||||
superWindowResolution();
|
||||
PatchLineComment c3 = clone(c1);
|
||||
c3.setWrittenOn(TimeUtil.nowTs());
|
||||
b1 = new ChangeBundle(c, messages(), patchSets(), approvals(), comments(c1),
|
||||
NOTE_DB);
|
||||
ChangeBundle b3 = new ChangeBundle(c, messages(), patchSets(), approvals(),
|
||||
comments(c3), REVIEW_DB);
|
||||
String msg = "writtenOn differs for PatchLineComment.Key " + c.getId()
|
||||
+ ",1,filename,uuid in NoteDb vs. ReviewDb:"
|
||||
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:10.0}";
|
||||
assertDiffs(b1, b3, msg);
|
||||
assertDiffs(b3, b1, msg);
|
||||
}
|
||||
|
||||
private static void assertNoDiffs(ChangeBundle a, ChangeBundle b) {
|
||||
assertThat(a.differencesFrom(b)).isEmpty();
|
||||
|
||||
Loading…
Reference in New Issue
Block a user