diff options
author | Duarte Meneses <duarte.meneses@sonarsource.com> | 2018-10-19 10:19:16 +0200 |
---|---|---|
committer | SonarTech <sonartech@sonarsource.com> | 2018-11-07 20:21:01 +0100 |
commit | 25ecd99833fa127875b91a80dc5b753c347c452b (patch) | |
tree | c50f3fa479ef28945a617ffe63e2487bba1a28a4 | |
parent | 1ff8a5c8a88f2b25cc20358e3e82aac9aa5aa1c6 (diff) | |
download | sonarqube-25ecd99833fa127875b91a80dc5b753c347c452b.tar.gz sonarqube-25ecd99833fa127875b91a80dc5b753c347c452b.zip |
SONAR-11367 Write duplication for changed files in PR and SLB
7 files changed, 168 insertions, 88 deletions
diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolder.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolder.java index 81c994fa726..642edd15483 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolder.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolder.java @@ -74,6 +74,14 @@ public interface TreeRootHolder { Optional<Component> getOptionalComponentByRef(int ref); /** + * Return a component from the report tree (see {@link #getReportTreeRoot()}, by its batch reference. + * + * @throws IllegalStateException if the holder is empty (ie. there is no root yet) + * @throws IllegalArgumentException if there's no {@link Component} with the specified reference + */ + Component getReportTreeComponentByRef(int ref); + + /** * Number of components, including root. * * @throws IllegalStateException if the holder is empty (ie. there is no root yet) diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolderImpl.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolderImpl.java index a0156ee1964..082fe9f86b0 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolderImpl.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/component/TreeRootHolderImpl.java @@ -35,6 +35,9 @@ public class TreeRootHolderImpl implements MutableTreeRootHolder { @CheckForNull private Map<Integer, Component> componentsByRef; + @CheckForNull + private Map<Integer, Component> extendedComponentsByRef; + private Component root; private Component extendedTreeRoot; @@ -77,12 +80,39 @@ public class TreeRootHolderImpl implements MutableTreeRootHolder { } @Override + public Component getReportTreeComponentByRef(int ref) { + checkInitialized(); + ensureExtendedComponentByRefIsPopulated(); + Component c = extendedComponentsByRef.get(ref); + if (c == null) { + throw new IllegalArgumentException(String.format("Component with ref '%s' can't be found", ref)); + } + return c; + } + + @Override public int getSize() { checkInitialized(); ensureComponentByRefIsPopulated(); return componentsByRef.size(); } + private void ensureExtendedComponentByRefIsPopulated() { + if (extendedComponentsByRef != null) { + return; + } + + final ImmutableMap.Builder<Integer, Component> builder = ImmutableMap.builder(); + new DepthTraversalTypeAwareCrawler( + new TypeAwareVisitorAdapter(CrawlerDepthLimit.FILE, POST_ORDER) { + @Override + public void visitAny(Component component) { + builder.put(component.getReportAttributes().getRef(), component); + } + }).visit(this.extendedTreeRoot); + this.extendedComponentsByRef = builder.build(); + } + private void ensureComponentByRefIsPopulated() { if (componentsByRef != null) { return; diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/duplication/DuplicationMeasures.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/duplication/DuplicationMeasures.java index b4c193ec945..d78c5a9dc9b 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/duplication/DuplicationMeasures.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/duplication/DuplicationMeasures.java @@ -76,7 +76,7 @@ public class DuplicationMeasures { public void execute() { new PathAwareCrawler<>( FormulaExecutorComponentVisitor.newBuilder(metricRepository, measureRepository).buildFor(formulas)) - .visit(treeRootHolder.getRoot()); + .visit(treeRootHolder.getRoot()); } protected DuplicationCounter createCounter() { diff --git a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/step/LoadDuplicationsFromReportStep.java b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/step/LoadDuplicationsFromReportStep.java index be7b456c4e0..bd3119ae4bc 100644 --- a/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/step/LoadDuplicationsFromReportStep.java +++ b/server/sonar-ce-task-projectanalysis/src/main/java/org/sonar/ce/task/projectanalysis/step/LoadDuplicationsFromReportStep.java @@ -63,7 +63,7 @@ public class LoadDuplicationsFromReportStep implements ComputationStep { @Override public void execute(ComputationStep.Context context) { DuplicationVisitor visitor = new DuplicationVisitor(); - new DepthTraversalTypeAwareCrawler(visitor).visit(treeRootHolder.getRoot()); + new DepthTraversalTypeAwareCrawler(visitor).visit(treeRootHolder.getReportTreeRoot()); context.getStatistics().add("duplications", visitor.count); } @@ -79,9 +79,8 @@ public class LoadDuplicationsFromReportStep implements ComputationStep { public Duplicate apply(@Nonnull ScannerReport.Duplicate input) { if (input.getOtherFileRef() != 0) { checkArgument(input.getOtherFileRef() != file.getReportAttributes().getRef(), "file and otherFile references can not be the same"); - return new InProjectDuplicate( - treeRootHolder.getComponentByRef(input.getOtherFileRef()), - convert(input.getRange())); + Component otherComponent = treeRootHolder.getReportTreeComponentByRef(input.getOtherFileRef()); + return new InProjectDuplicate(otherComponent, convert(input.getRange())); } return new InnerDuplicate(convert(input.getRange())); } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/cpd/CpdExecutor.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/cpd/CpdExecutor.java index 878cf807324..464241ab670 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/cpd/CpdExecutor.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/cpd/CpdExecutor.java @@ -20,9 +20,11 @@ package org.sonar.scanner.cpd; import com.google.common.annotations.VisibleForTesting; +import java.util.ArrayList; import java.util.Collection; import java.util.Iterator; import java.util.List; +import java.util.Optional; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; @@ -34,6 +36,7 @@ import java.util.stream.Collectors; import org.sonar.api.batch.fs.InputComponent; import org.sonar.api.batch.fs.InputFile; import org.sonar.api.batch.fs.internal.DefaultInputComponent; +import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.utils.log.Logger; import org.sonar.api.utils.log.Loggers; import org.sonar.duplications.block.Block; @@ -66,15 +69,21 @@ public class CpdExecutor { private final InputComponentStore componentStore; private final ProgressReport progressReport; private final CpdSettings settings; - private int count; + private final ExecutorService executorService; + private int count = 0; private int total; public CpdExecutor(CpdSettings settings, SonarCpdBlockIndex index, ReportPublisher publisher, InputComponentStore inputComponentCache) { + this(settings, index, publisher, inputComponentCache, Executors.newSingleThreadExecutor()); + } + + public CpdExecutor(CpdSettings settings, SonarCpdBlockIndex index, ReportPublisher publisher, InputComponentStore inputComponentCache, ExecutorService executorService) { this.settings = settings; this.index = index; this.publisher = publisher; this.componentStore = inputComponentCache; this.progressReport = new ProgressReport("CPD computation", TimeUnit.SECONDS.toMillis(10)); + this.executorService = executorService; } public void execute() { @@ -83,19 +92,28 @@ public class CpdExecutor { @VisibleForTesting void execute(long timeout) { - total = index.noResources(); - int filesWithoutBlocks = index.noIndexedFiles() - total; + List<FileBlocks> components = new ArrayList<>(index.noResources()); + Iterator<ResourceBlocks> it = index.iterator(); + + while (it.hasNext()) { + ResourceBlocks resourceBlocks = it.next(); + Optional<FileBlocks> fileBlocks = toFileBlocks(resourceBlocks.resourceId(), resourceBlocks.blocks()); + if (!fileBlocks.isPresent() || fileBlocks.get().getInputFile().status() == InputFile.Status.SAME) { + continue; + } + components.add(fileBlocks.get()); + } + + int filesWithoutBlocks = index.noIndexedFiles() - index.noResources(); if (filesWithoutBlocks > 0) { LOG.info("{} {} had no CPD blocks", filesWithoutBlocks, pluralize(filesWithoutBlocks)); } + + total = components.size(); progressReport.start(String.format("Calculating CPD for %d %s", total, pluralize(total))); - ExecutorService executorService = Executors.newSingleThreadExecutor(); try { - Iterator<ResourceBlocks> it = index.iterator(); - - while (it.hasNext()) { - ResourceBlocks resourceBlocks = it.next(); - runCpdAnalysis(executorService, resourceBlocks.resourceId(), resourceBlocks.blocks(), timeout); + for (FileBlocks fileBlocks : components) { + runCpdAnalysis(executorService, fileBlocks.getInputFile(), fileBlocks.getBlocks(), timeout); count++; } progressReport.stop("CPD calculation finished"); @@ -112,18 +130,7 @@ public class CpdExecutor { } @VisibleForTesting - void runCpdAnalysis(ExecutorService executorService, String componentKey, final Collection<Block> fileBlocks, long timeout) { - DefaultInputComponent component = (DefaultInputComponent) componentStore.getByKey(componentKey); - if (component == null) { - LOG.error("Resource not found in component store: {}. Skipping CPD computation for it", componentKey); - return; - } - - InputFile inputFile = (InputFile) component; - if (inputFile.status() == InputFile.Status.SAME) { - return; - } - + void runCpdAnalysis(ExecutorService executorService, DefaultInputFile inputFile, Collection<Block> fileBlocks, long timeout) { LOG.debug("Detection of duplications for {}", inputFile.absolutePath()); progressReport.message(String.format("%d/%d - current file: %s", count, total, inputFile.absolutePath())); @@ -150,7 +157,7 @@ public class CpdExecutor { filtered = duplications; } - saveDuplications(component, filtered); + saveDuplications(inputFile, filtered); } @VisibleForTesting final void saveDuplications(final DefaultInputComponent component, List<CloneGroup> duplications) { @@ -173,6 +180,15 @@ public class CpdExecutor { publisher.getWriter().writeComponentDuplications(component.batchId(), reportDuplications); } + private Optional<FileBlocks> toFileBlocks(String componentKey, Collection<Block> fileBlocks) { + DefaultInputFile component = (DefaultInputFile) componentStore.getByKey(componentKey); + if (component == null) { + LOG.error("Resource not found in component store: {}. Skipping CPD computation for it", componentKey); + return Optional.empty(); + } + return Optional.of(new FileBlocks(component, fileBlocks)); + } + private Duplication toReportDuplication(InputComponent component, Duplication.Builder dupBuilder, Duplicate.Builder blockBuilder, CloneGroup input) { dupBuilder.clear(); ClonePart originBlock = input.getOriginPart(); @@ -207,4 +223,22 @@ public class CpdExecutor { } return dupBuilder.build(); } + + private static class FileBlocks { + public FileBlocks(DefaultInputFile inputFile, Collection<Block> blocks) { + this.inputFile = inputFile; + this.blocks = blocks; + } + + private DefaultInputFile inputFile; + private Collection<Block> blocks; + + public DefaultInputFile getInputFile() { + return inputFile; + } + + public Collection<Block> getBlocks() { + return blocks; + } + } } diff --git a/sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java b/sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java index 88f47c292ab..b410ed19c31 100644 --- a/sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java +++ b/sonar-scanner-engine/src/main/java/org/sonar/scanner/sensor/DefaultSensorStorage.java @@ -506,9 +506,6 @@ public class DefaultSensorStorage implements SensorStorage { @Override public void store(DefaultCpdTokens defaultCpdTokens) { DefaultInputFile inputFile = (DefaultInputFile) defaultCpdTokens.inputFile(); - if (shouldSkipStorage(inputFile)) { - return; - } inputFile.setPublished(true); PmdBlockChunker blockChunker = new PmdBlockChunker(getCpdBlockSize(inputFile.language())); List<Block> blocks = blockChunker.chunk(inputFile.key(), defaultCpdTokens.getTokenLines()); diff --git a/sonar-scanner-engine/src/test/java/org/sonar/scanner/cpd/CpdExecutorTest.java b/sonar-scanner-engine/src/test/java/org/sonar/scanner/cpd/CpdExecutorTest.java index 732f36e2696..929afe6a7c0 100644 --- a/sonar-scanner-engine/src/test/java/org/sonar/scanner/cpd/CpdExecutorTest.java +++ b/sonar-scanner-engine/src/test/java/org/sonar/scanner/cpd/CpdExecutorTest.java @@ -25,7 +25,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; import java.util.function.Consumer; import javax.annotation.Nullable; import org.junit.Before; @@ -33,6 +36,7 @@ import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.rules.TemporaryFolder; +import org.mockito.ArgumentMatchers; import org.sonar.api.batch.fs.InputFile; import org.sonar.api.batch.fs.internal.DefaultInputFile; import org.sonar.api.batch.fs.internal.DefaultInputModule; @@ -55,7 +59,8 @@ import org.sonar.scanner.scan.filesystem.InputComponentStore; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; public class CpdExecutorTest { @@ -69,6 +74,7 @@ public class CpdExecutorTest { public ExpectedException thrown = ExpectedException.none(); private CpdExecutor executor; + private ExecutorService executorService = mock(ExecutorService.class); private CpdSettings settings = mock(CpdSettings.class); private ReportPublisher publisher = mock(ReportPublisher.class); private SonarCpdBlockIndex index = new SonarCpdBlockIndex(publisher, settings); @@ -88,7 +94,7 @@ public class CpdExecutorTest { DefaultInputModule inputModule = TestInputFileBuilder.newDefaultInputModule("foo", baseDir); componentStore = new InputComponentStore(inputModule, mock(BranchConfiguration.class)); - executor = new CpdExecutor(settings, index, publisher, componentStore); + executor = new CpdExecutor(settings, index, publisher, componentStore, executorService); reader = new ScannerReportReader(outputDir); batchComponent1 = createComponent("src/Foo.php", 5); @@ -97,30 +103,16 @@ public class CpdExecutorTest { batchComponent4 = createComponent("src/Foo4.php", 5, f -> f.setStatus(InputFile.Status.SAME)); } - private DefaultInputFile createComponent(String relativePath, int lines) { - return createComponent(relativePath, lines, f -> { - }); - } - - private DefaultInputFile createComponent(String relativePath, int lines, Consumer<TestInputFileBuilder> config) { - TestInputFileBuilder fileBuilder = new TestInputFileBuilder("foo", relativePath) - .setModuleBaseDir(baseDir.toPath()) - .setLines(lines); - config.accept(fileBuilder); - DefaultInputFile file = fileBuilder.build(); - componentStore.put(file); - return file; - } - @Test - public void testNothingToSave() { + public void dont_fail_if_nothing_to_save() { executor.saveDuplications(batchComponent1, Collections.<CloneGroup>emptyList()); assertThat(reader.readComponentDuplications(batchComponent1.batchId())).hasSize(0); } @Test - public void reportOneSimpleDuplicationBetweenTwoFiles() { - List<CloneGroup> groups = Collections.singletonList(newCloneGroup(new ClonePart(batchComponent1.key(), 0, 2, 4), + public void should_save_single_duplication() { + List<CloneGroup> groups = Collections.singletonList(newCloneGroup( + new ClonePart(batchComponent1.key(), 0, 2, 4), new ClonePart(batchComponent2.key(), 0, 15, 17))); executor.saveDuplications(batchComponent1, groups); @@ -130,17 +122,10 @@ public class CpdExecutorTest { } @Test - public void dontReportDuplicationOnUnmodifiedFileInSLB() { - ExecutorService executorService = mock(ExecutorService.class); - executor.runCpdAnalysis(executorService, batchComponent4.key(), Collections.emptyList(), 1000L); - - readDuplications(batchComponent4, 0); - verifyZeroInteractions(executorService); - } - - @Test - public void reportDuplicationOnSameFile() { - List<CloneGroup> groups = Collections.singletonList(newCloneGroup(new ClonePart(batchComponent1.key(), 0, 5, 204), new ClonePart(batchComponent1.key(), 0, 215, 414))); + public void should_save_duplication_on_same_file() { + List<CloneGroup> groups = Collections.singletonList(newCloneGroup( + new ClonePart(batchComponent1.key(), 0, 5, 204), + new ClonePart(batchComponent1.key(), 0, 215, 414))); executor.saveDuplications(batchComponent1, groups); Duplication[] dups = readDuplications(1); @@ -148,13 +133,13 @@ public class CpdExecutorTest { } @Test - public void reportTooManyDuplicates() { + public void should_limit_number_of_references() { // 1 origin part + 101 duplicates = 102 List<ClonePart> parts = new ArrayList<>(CpdExecutor.MAX_CLONE_PART_PER_GROUP + 2); for (int i = 0; i < CpdExecutor.MAX_CLONE_PART_PER_GROUP + 2; i++) { parts.add(new ClonePart(batchComponent1.key(), i, i, i + 1)); } - List<CloneGroup> groups = Arrays.asList(CloneGroup.builder().setLength(0).setOrigin(parts.get(0)).setParts(parts).build()); + List<CloneGroup> groups = Collections.singletonList(CloneGroup.builder().setLength(0).setOrigin(parts.get(0)).setParts(parts).build()); executor.saveDuplications(batchComponent1, groups); Duplication[] dups = readDuplications(1); @@ -166,7 +151,7 @@ public class CpdExecutorTest { } @Test - public void reportTooManyDuplications() throws Exception { + public void should_limit_number_of_clones() { // 1 origin part + 101 duplicates = 102 List<CloneGroup> dups = new ArrayList<>(CpdExecutor.MAX_CLONE_GROUP_PER_FILE + 1); for (int i = 0; i < CpdExecutor.MAX_CLONE_GROUP_PER_FILE + 1; i++) { @@ -183,9 +168,11 @@ public class CpdExecutorTest { } @Test - public void reportOneDuplicatedGroupInvolvingMoreThanTwoFiles() throws Exception { - List<CloneGroup> groups = Collections.singletonList(newCloneGroup(new ClonePart(batchComponent1.key(), 0, 5, 204), - new ClonePart(batchComponent2.key(), 0, 15, 214), new ClonePart(batchComponent3.key(), 0, 25, 224))); + public void should_save_duplication_involving_three_files() { + List<CloneGroup> groups = Collections.singletonList(newCloneGroup( + new ClonePart(batchComponent1.key(), 0, 5, 204), + new ClonePart(batchComponent2.key(), 0, 15, 214), + new ClonePart(batchComponent3.key(), 0, 25, 224))); executor.saveDuplications(batchComponent1, groups); Duplication[] dups = readDuplications(1); @@ -195,7 +182,7 @@ public class CpdExecutorTest { } @Test - public void reportTwoDuplicatedGroupsInvolvingThreeFiles() throws Exception { + public void should_save_two_duplicated_groups_involving_three_files() { List<CloneGroup> groups = Arrays.asList( newCloneGroup(new ClonePart(batchComponent1.key(), 0, 5, 204), new ClonePart(batchComponent2.key(), 0, 15, 214)), @@ -209,36 +196,61 @@ public class CpdExecutorTest { } @Test - public void failOnMissingComponent() { - executor.runCpdAnalysis(null, "unknown", Collections.emptyList(), 1); - readDuplications(0); + public void should_ignore_unmodified_files_in_SLB() { + Block block = Block.builder().setBlockHash(new ByteArray("AAAABBBBCCCC")).build(); + index.insert(batchComponent4, Collections.singletonList(block)); + executor.execute(); + + verify(executorService).shutdown(); + verifyNoMoreInteractions(executorService); + readDuplications(batchComponent4, 0); + } + + @Test + public void should_ignore_missing_component() { + Block block = Block.builder() + .setBlockHash(new ByteArray("AAAABBBBCCCC")) + .setResourceId("unknown") + .build(); + index.insert(batchComponent1, Collections.singletonList(block)); + executor.execute(); + + verify(executorService).shutdown(); + verifyNoMoreInteractions(executorService); + readDuplications(batchComponent1, 0); assertThat(logTester.logs(LoggerLevel.ERROR)).contains("Resource not found in component store: unknown. Skipping CPD computation for it"); } @Test - public void timeout() { - for (int i = 1; i <= 2; i++) { - DefaultInputFile component = createComponent("src/Foo" + i + ".php", 100); - List<Block> blocks = new ArrayList<>(); - for (int j = 1; j <= 10000; j++) { - blocks.add(Block.builder() - .setResourceId(component.key()) - .setIndexInFile(j) - .setLines(j, j + 1) - .setUnit(j, j + 1) - .setBlockHash(new ByteArray("abcd1234".getBytes())) - .build()); - } - index.insert(component, blocks); - } + public void should_timeout() { + Block block = Block.builder() + .setBlockHash(new ByteArray("AAAABBBBCCCC")) + .setResourceId(batchComponent1.key()) + .build(); + index.insert(batchComponent1, Collections.singletonList(block)); + when(executorService.submit(ArgumentMatchers.any(Callable.class))).thenReturn(new CompletableFuture()); executor.execute(1); readDuplications(0); assertThat(logTester.logs(LoggerLevel.WARN)) .usingElementComparator((l, r) -> l.matches(r) ? 0 : 1) .containsOnly( - "Timeout during detection of duplications for .*Foo1.php", - "Timeout during detection of duplications for .*Foo2.php"); + "Timeout during detection of duplications for .*Foo.php"); + } + + private DefaultInputFile createComponent(String relativePath, int lines) { + return createComponent(relativePath, lines, f -> { + }); + } + + private DefaultInputFile createComponent(String relativePath, int lines, Consumer<TestInputFileBuilder> config) { + TestInputFileBuilder fileBuilder = new TestInputFileBuilder("foo", relativePath) + .setModuleBaseDir(baseDir.toPath()) + .setLines(lines); + config.accept(fileBuilder); + DefaultInputFile file = fileBuilder.build(); + componentStore.put(file); + return file; } private Duplication[] readDuplications(int expected) { |