Skip to content

Commit

Permalink
Fix some quality flaws and improve coverage
Browse files Browse the repository at this point in the history
  • Loading branch information
@henryju
henryju committed Feb 18, 2016
1 parent 17ffd3b commit a1e2c6d
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 59 deletions.
113 changes: 71 additions & 42 deletions src/main/java/org/sonar/plugins/scm/perforce/PerforceBlameCommand.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,21 @@
import com.perforce.p4java.core.file.IFileAnnotation;
import com.perforce.p4java.core.file.IFileRevisionData;
import com.perforce.p4java.core.file.IFileSpec;
import com.perforce.p4java.exception.AccessException;
import com.perforce.p4java.exception.ConnectionException;
import com.perforce.p4java.exception.P4JavaException;
import com.perforce.p4java.exception.RequestException;
import com.perforce.p4java.impl.generic.core.file.FileSpec;
import com.perforce.p4java.option.server.GetFileAnnotationsOptions;
import com.perforce.p4java.option.server.GetRevisionHistoryOptions;
import com.perforce.p4java.server.IOptionsServer;

import java.util.*;
import java.util.ArrayList;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand All @@ -44,8 +52,8 @@ public class PerforceBlameCommand extends BlameCommand {

private static final Logger LOG = LoggerFactory.getLogger(PerforceBlameCommand.class);
private final PerforceConfiguration config;
private final Map<Integer, IFileRevisionData> revisionDataMap = new HashMap<>();
private final Map<Integer, IChangelist> changelistMap = new HashMap<>();
private final Map<Integer, IFileRevisionData> revisionDataByChangelistId = new HashMap<>();
private final Map<Integer, IChangelist> changelistCache = new HashMap<>();

public PerforceBlameCommand(PerforceConfiguration config) {
this.config = config;
Expand Down Expand Up @@ -89,59 +97,80 @@ void blame(InputFile inputFile, IOptionsServer server, BlameOutput output) throw
return;
}
for (IFileRevisionData revisionData : entry.getValue()) {
revisionDataMap.put(revisionData.getChangelistId(), revisionData);
revisionDataByChangelistId.put(revisionData.getChangelistId(), revisionData);
}
}

// Compute blame, getting changelist from server if not already retrieved
List<BlameLine> lines = new ArrayList<>();
for (IFileAnnotation fileAnnotation : fileAnnotations) {
int lowerChangelistId = fileAnnotation.getLower();
List<BlameLine> lines = computeBlame(inputFile, server, fileAnnotations);

IFileRevisionData data = revisionDataMap.get(lowerChangelistId);
if (data != null) {
// SONARPLUGINS-3097: Perforce does not report blame on last empty line, so populate from last line with blame
if (lines.size() == (inputFile.lines() - 1)) {
lines.add(lines.get(lines.size() - 1));
}

lines.add(new BlameLine()
.revision(String.valueOf(lowerChangelistId))
.date(data.getDate())
.author(data.getUserName()));
output.blameResult(inputFile, lines);
}

} else {
// Sometimes they're missing from the revision history, so try to get it directly.
/**
* Compute blame, getting changelist from server if not already retrieved
*/
private List<BlameLine> computeBlame(InputFile inputFile, IOptionsServer server, List<IFileAnnotation> fileAnnotations)
throws ConnectionException, RequestException, AccessException {
List<BlameLine> lines = new ArrayList<>();
for (IFileAnnotation fileAnnotation : fileAnnotations) {
int lowerChangelistId = fileAnnotation.getLower();

BlameLine blameLine = blameLineFromHistory(lowerChangelistId);
if (blameLine == null) {
LOG.debug("Changelist " + lowerChangelistId + " was not found in history for " + inputFile + ". It will be fetched directly.");
IChangelist changelist = changelistMap.get(lowerChangelistId);
if (changelist == null) {
changelist = server.getChangelist(lowerChangelistId);
if (changelist != null) { // sometimes even that can fail due to cross-server imports
changelistMap.put(lowerChangelistId, changelist);
}
}

if (changelist != null) {
lines.add(new BlameLine()
.revision(String.valueOf(lowerChangelistId))
.date(changelist.getDate())
.author(changelist.getUsername()));
} else {
// We really couldn't get any information for this changelist!
// Unfortunately, blame information is required for every line...
lines.add(new BlameLine()
.revision(String.valueOf(lowerChangelistId))
.date(new Date(0))
.author("unknown"));
}
blameLine = blameLineFromChangeListDetails(server, lowerChangelistId);
}

if (blameLine == null) {
// We really couldn't get any information for this changelist!
// Unfortunately, blame information is required for every line...
blameLine = new BlameLine()
.revision(String.valueOf(lowerChangelistId))
.date(new Date(0))
.author("unknown");
}

lines.add(blameLine);
}
return lines;
}

// SONARPLUGINS-3097: Perforce does not report blame on last empty line, so populate from last line with blame
if (lines.size() == (inputFile.lines() - 1)) {
lines.add(lines.get(lines.size() - 1));
@CheckForNull
private BlameLine blameLineFromChangeListDetails(IOptionsServer server, int changelistId)
throws ConnectionException, RequestException, AccessException {
IChangelist changelist = changelistCache.get(changelistId);
if (changelist == null) {
changelist = server.getChangelist(changelistId);
// sometimes even that can fail due to cross-server imports
if (changelist != null) {
changelistCache.put(changelistId, changelist);
}
}

output.blameResult(inputFile, lines);
if (changelist != null) {
return new BlameLine()
.revision(String.valueOf(changelistId))
.date(changelist.getDate())
.author(changelist.getUsername());
}
return null;
}

@CheckForNull
private BlameLine blameLineFromHistory(int changelistId) {
IFileRevisionData data = revisionDataByChangelistId.get(changelistId);
if (data != null) {
return new BlameLine()
.revision(String.valueOf(changelistId))
.date(data.getDate())
.author(data.getUserName());
}
return null;
}

/**
Expand Down
57 changes: 40 additions & 17 deletions src/test/java/org/sonar/plugins/scm/perforce/PerforceBlameCommandTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,24 @@
import com.perforce.p4java.option.server.GetFileAnnotationsOptions;
import com.perforce.p4java.option.server.GetRevisionHistoryOptions;
import com.perforce.p4java.server.IOptionsServer;
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.junit.Test;
import org.sonar.api.batch.fs.InputFile;
import org.sonar.api.batch.scm.BlameCommand.BlameOutput;
import org.sonar.api.batch.scm.BlameLine;

import java.util.*;

import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyListOf;
import static org.mockito.Mockito.*;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;

public class PerforceBlameCommandTest {

Expand All @@ -62,21 +70,32 @@ public void testBlameSubmittedFile() throws Exception {
IOptionsServer server = mock(IOptionsServer.class);
PerforceBlameCommand command = new PerforceBlameCommand(mock(PerforceConfiguration.class));

IFileAnnotation annotation1 = mock(IFileAnnotation.class);
when(annotation1.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation1.getLower()).thenReturn(3);
// Changelist 3 is present in history

IFileAnnotation line1ChangeList3 = mock(IFileAnnotation.class);
when(line1ChangeList3.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(line1ChangeList3.getLower()).thenReturn(3);

IFileAnnotation line2ChangeList3 = mock(IFileAnnotation.class);
when(line2ChangeList3.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(line2ChangeList3.getLower()).thenReturn(3);

IFileAnnotation annotation2 = mock(IFileAnnotation.class);
when(annotation2.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation2.getLower()).thenReturn(3);
// Changelist 4 is not present in history but can be fetched from server

IFileAnnotation annotation3 = mock(IFileAnnotation.class);
when(annotation3.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation3.getLower()).thenReturn(4);
IFileAnnotation line3ChangeList4 = mock(IFileAnnotation.class);
when(line3ChangeList4.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(line3ChangeList4.getLower()).thenReturn(4);

IFileAnnotation annotation4 = mock(IFileAnnotation.class);
when(annotation4.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(annotation4.getLower()).thenReturn(5);
// Changelist 5 is not present in history nor in server

IFileAnnotation line4ChangeList5 = mock(IFileAnnotation.class);
when(line4ChangeList5.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(line4ChangeList5.getLower()).thenReturn(5);

// Put Changlist 4 again to verify we fetch only once from server
IFileAnnotation line5ChangeList4 = mock(IFileAnnotation.class);
when(line5ChangeList4.getDepotPath()).thenReturn("foo/bar/src/Foo.java");
when(line5ChangeList4.getLower()).thenReturn(4);

Map<IFileSpec, List<IFileRevisionData>> result = new HashMap<>();
IFileSpec fileSpecResult = mock(IFileSpec.class);
Expand All @@ -89,7 +108,8 @@ public void testBlameSubmittedFile() throws Exception {
result.put(fileSpecResult, Collections.singletonList(revision3));

when(server.getRevisionHistory(anyListOf(IFileSpec.class), any(GetRevisionHistoryOptions.class))).thenReturn(result);
when(server.getFileAnnotations(anyListOf(IFileSpec.class), any(GetFileAnnotationsOptions.class))).thenReturn(Arrays.asList(annotation1, annotation2, annotation3, annotation4));
when(server.getFileAnnotations(anyListOf(IFileSpec.class), any(GetFileAnnotationsOptions.class)))
.thenReturn(Arrays.asList(line1ChangeList3, line2ChangeList3, line3ChangeList4, line4ChangeList5, line5ChangeList4));

IChangelist changelist = mock(IChangelist.class);
when(changelist.getDate()).thenReturn(date);
Expand All @@ -105,7 +125,10 @@ public void testBlameSubmittedFile() throws Exception {
BlameLine line2 = new BlameLine().revision("3").date(date).author("jhenry");
BlameLine line3 = new BlameLine().revision("4").date(date).author("bgates");
BlameLine line4 = new BlameLine().revision("5").date(new Date(0)).author("unknown");
verify(blameOutput).blameResult(inputFile, Arrays.asList(line1, line2, line3, line4));
BlameLine line5 = new BlameLine().revision("4").date(date).author("bgates");
verify(blameOutput).blameResult(inputFile, Arrays.asList(line1, line2, line3, line4, line5));

// Changelist 4 should have been fetched only once
verify(server, times(1)).getChangelist(4);
}

Expand Down

0 comments on commit a1e2c6d

Please sign in to comment.