Fix and test of SonarOpenCommunity#405.

Do not include external files in package/file graphs, this causes the dependency analyzer to extend projects scope by pulling the dependencies into it.
francoisferrand · Jan 30, 2015 · fcfd860 · fcfd860
1 parent 6f32c49
commit fcfd860
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 20 deletions.
diff --git a/integration-tests/features/smoketest.feature b/integration-tests/features/smoketest.feature
@@ -12,12 +12,14 @@ Feature: Smoketest
               .*WARN.*cannot find the sources for '#include <gtest/gtest\.h>'
               .*WARN.*cannot find the sources for '#include <iostream>'
               .*WARN  - Cannot find the file '.*component_XXX.cc', skipping violations
+              .*WARN  - Already created edge from 'src/cli/main.cc' \(line 2\) to 'src/lib/component1.hh', previous edge from line 4
+              .*WARN  - Already created edge from 'src/cli/main.cc' \(line 5\) to '3rdparty/extlib.hh', previous edge from line 3
               """
           AND the following metrics have following values:
                | metric                   | value |
                # size metrics
                | ncloc                    | 56    |
-               | lines                    | 148   |
+               | lines                    | 151   |
                | statements               | 36    |
                | classes                  | 1     |
                | files                    | 8     |
@@ -27,7 +29,7 @@ Feature: Smoketest
                | comment_lines_density    | 30    |
                | comment_lines            | 24    |
                # duplications
-               | duplicated_lines_density | 58.1  |
+               | duplicated_lines_density | 57.0  |
                | duplicated_lines         | 86    |
                | duplicated_blocks        | 2     |
                | duplicated_files         | 2     |

diff --git a/integration-tests/testdata/smoketest_project/src/cli/main.cc b/integration-tests/testdata/smoketest_project/src/cli/main.cc
@@ -1,6 +1,8 @@
 #include <iostream>
 #include <lib/component1.hh>
 #include <extlib.hh>
+#include <lib/component1.hh>
+#include <extlib.hh>
 
 int main(int argc, char* argv[])
 {

diff --git a/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/squid/DependencyAnalyzer.java b/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/squid/DependencyAnalyzer.java
@@ -86,26 +86,28 @@ public void addFile(File sonarFile, Collection<CxxPreprocessor.Include> included
     directoryFiles.put(sonarDir, sonarFile);
 
     //Build the dependency graph
+    Map<String, Integer> firstIncludeLine = new HashMap<String, Integer>();
     for (CxxPreprocessor.Include include : includedFiles) {
       File includedFile = File.fromIOFile(new java.io.File(include.getPath()), project);
-      if (includedFile == null) {
-        CxxUtils.LOG.warn("Unable to find resource '" + include.getPath() + "' to create a dependency with '" + sonarFile.getKey() + "'");
-      } else if (filesGraph.hasEdge(sonarFile, includedFile)) {
-        FileEdge fileEdge = filesGraph.getEdge(sonarFile, includedFile);
+      String includedFilePath = includedFile != null ? includedFile.getPath() : include.getPath();
+      Integer prevIncludeLine = firstIncludeLine.put(includedFilePath, include.getLine());
+      if (prevIncludeLine != null) {
         Issuable issuable = perspectives.as(Issuable.class, sonarFile);
         if ((issuable != null) && (duplicateIncludeRule != null)) {
           Issue issue = issuable.newIssueBuilder()
               .ruleKey(duplicateIncludeRule.ruleKey())
               .line(include.getLine())
-              .message("Remove duplicated include, \"" + includedFile.getLongName() + "\" is already included at line " + fileEdge.getLine() + ".")
+              .message("Remove duplicated include, \"" + includedFilePath + "\" is already included at line " + prevIncludeLine + ".")
               .build();
           if (issuable.addIssue(issue))
             violationsCount++;
         } else {
-          CxxUtils.LOG.warn("Already created edge from '" + sonarFile.getKey() + "' (line " + include.getLine() + ") to '" + includedFile.getKey() + "'" +
-              ", previous edge from line " + fileEdge.getLine());
+          CxxUtils.LOG.warn("Already created edge from '" + sonarFile.getKey() + "' (line " + include.getLine() + ") to '" + includedFilePath + "'" +
+              ", previous edge from line " + prevIncludeLine);
         }
-      } else {
+      } else if (includedFile == null) {
+        CxxUtils.LOG.warn("Unable to find resource '" + include.getPath() + "' to create a dependency with '" + sonarFile.getKey() + "'");
+      } else if (context.isIndexed(includedFile, false)) {
         //Add the dependency in the files graph
         FileEdge fileEdge = new FileEdge(sonarFile, includedFile, include.getLine());
         filesGraph.addEdge(fileEdge);
@@ -120,6 +122,8 @@ public void addFile(File sonarFile, Collection<CxxPreprocessor.Include> included
           }
           edge.addRootEdge(fileEdge);
         }
+      }  else {
+        CxxUtils.LOG.debug("Skipping dependency to file '{}', because it is'nt part of this project", includedFile.getName());
       }
     }
   }
@@ -131,20 +135,15 @@ public void save() {
     for (Directory dir : packages) {
       //Save dependencies (cross-directories, including cross-directory file dependencies)
       for (DirectoryEdge edge : packagesGraph.getOutgoingEdges(dir)) {
-        Directory to = edge.getTo();
-        if(context.isIndexed(to, false)){
-          Dependency dependency = new Dependency(dir, to)
+        Dependency dependency = new Dependency(dir, edge.getTo())
             .setUsage("references")
             .setWeight(edge.getWeight())
             .setParent(null);
-          context.saveDependency(dependency);
-          dependencyIndex.put(edge, dependency);
+        context.saveDependency(dependency);
+        dependencyIndex.put(edge, dependency);
 
-          for(FileEdge subEdge : edge.getRootEdges()) {
-            saveFileEdge(subEdge, dependency);
-          }
-        } else {
-          CxxUtils.LOG.debug("Skipping dependency to directory '{}', because it is'nt part of this project", to.getName());
+        for(FileEdge subEdge : edge.getRootEdges()) {
+          saveFileEdge(subEdge, dependency);
         }
       }
 

diff --git a/sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/squid/CxxSquidSensorTest.java b/sonar-cxx-plugin/src/test/java/org/sonar/plugins/cxx/squid/CxxSquidSensorTest.java
@@ -25,6 +25,9 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.anyBoolean;
 
 import java.io.File;
 import java.util.ArrayList;
@@ -40,6 +43,7 @@
 import org.sonar.api.measures.CoreMetrics;
 import org.sonar.api.resources.Directory;
 import org.sonar.api.resources.Project;
+import org.sonar.api.resources.Resource;
 import org.sonar.api.scan.filesystem.ModuleFileSystem;
 import org.sonar.plugins.cxx.CxxPlugin;
 import org.sonar.plugins.cxx.TestUtils;
@@ -57,6 +61,7 @@ public void setUp() {
     emptyList = new ArrayList<File>();
     settings = new Settings();
     context = mock(SensorContext.class);
+    when(context.isIndexed(any(Resource.class), anyBoolean())).thenReturn(true);
   }
 
   @Test