Improve JaCoCo parsing and improve Kotlin support (#1326)

The jacoco parser made some relatively safe assumptions about the jacoco report, except when jacoco is used to instrument kotlin. Kotlin doesn't have the same restrictions as java when it comes to top level classes, so the report parsing will just immediately error out.

This improves the parser by making it sourcefilename centric instead of class centric (falling back on the pattern of top level class + .java == sourcefilename if the sourcefilename is missing).

In addition, when aggregated across multiple projects, the report commonly doesn't have the full relative paths, so we need to be able to find both java and kotlin files. There is also an assumption that a fully qualified class can only exist in one location and it would panic; however, this isn't entirely uncommon, so don't panic, but instead, inform the user that its not fully supported.

Fixes #1279

Co-authored-by: Jason Gilanfarr <[email protected]>

Author: AadityaDhingra

src/parser.rs

@@ -742,7 +742,6 @@ fn parse_jacoco_report_package<T: BufRead>(
             Ok(Event::Start(ref e)) => {
                 match e.local_name().into_inner() {
                     b"class" => {
-                        // Fully qualified class name: "org/example/Person$Age"
                         let fq_class = get_xml_attribute(parser, e, "name")?;
                         // Class name: "Person$Age"
                         let class = fq_class
@@ -754,11 +753,16 @@ fn parse_jacoco_report_package<T: BufRead>(
                             .split('$')
                             .next()
                             .expect("Failed to parse top class name");
+                        // Fully qualified class name: "org/example/Person$Age"
+                        // Generally, we will use the filename if its present,
+                        // but if it isn't, fallback to the top level class name
+                        let file = get_xml_attribute(parser, e, "sourcefilename")
+                            .unwrap_or(format!("{}.java", top_class));
 
                         // Process all <method /> and <counter /> for this class
                         let functions = parse_jacoco_report_class(parser, buf, class)?;
 
-                        match results_map.entry(top_class.to_string()) {
+                        match results_map.entry(file.to_string()) {
                             hash_map::Entry::Occupied(obj) => {
                                 obj.into_mut().functions.extend(functions);
                             }
@@ -772,12 +776,13 @@ fn parse_jacoco_report_package<T: BufRead>(
                         };
                     }
                     b"sourcefile" => {
-                        let sourcefile = get_xml_attribute(parser, e, "name")?;
-                        let class = sourcefile.trim_end_matches(".java");
+                        // Fully qualified class name: "org/example/Person$Age"
+                        let file = get_xml_attribute(parser, e, "name")?;
+
                         let JacocoReport { lines, branches } =
                             parse_jacoco_report_sourcefile(parser, buf)?;
 
-                        match results_map.entry(class.to_string()) {
+                        match results_map.entry(file.to_string()) {
                             hash_map::Entry::Occupied(obj) => {
                                 let obj = obj.into_mut();
                                 obj.lines = lines;
@@ -801,23 +806,14 @@ fn parse_jacoco_report_package<T: BufRead>(
         }
     }
 
-    for (class, result) in &results_map {
-        if result.lines.is_empty() && result.branches.is_empty() {
-            return Err(ParserError::InvalidData(format!(
-                "Class {}/{} is not the top class in its file.",
-                package, class
-            )));
-        }
-    }
-
     // Change all keys from the class name to the file name and turn the result into a Vec.
     // If package is the empty string, we have to trim the leading '/' in order to obtain a
     // relative path.
     Ok(results_map
         .into_iter()
         .map(|(class, result)| {
             (
-                format!("{}/{}.java", package, class)
+                format!("{}/{}", package, class)
                     .trim_start_matches('/')
                     .to_string(),
                 result,
@@ -2089,20 +2085,76 @@ TN:http_3a_2f_2fweb_2dplatform_2etest_3a8000_2freferrer_2dpolicy_2fgen_2fsrcdoc_
     }
 
     #[test]
-    #[should_panic]
-    fn test_parser_jacoco_xml_non_top_level_classes_panics() {
-        let f = File::open("./test/jacoco/multiple-top-level-classes.xml")
-            .expect("Failed to open xml file");
-        let file = BufReader::new(&f);
-        let _results = parse_jacoco_xml_report(file).unwrap();
-    }
+    fn test_parser_jacoco_kotlin() {
+        let mut lines: BTreeMap<u32, u64> = BTreeMap::new();
+        for i in &[
+            (5, 0),
+            (9, 0),
+            (14, 0),
+            (27, 0),
+            (30, 0),
+            (32, 0),
+            (41, 0),
+            (49, 0),
+            (57, 0),
+            (65, 0),
+            (73, 0),
+            (81, 0),
+            (89, 0),
+            (97, 0),
+            (104, 0),
+            (105, 0),
+            (106, 0),
+            (107, 0),
+            (108, 0),
+            (109, 0),
+            (110, 0),
+            (111, 0),
+            (112, 0),
+            (118, 0),
+            (119, 0),
+            (120, 0),
+        ] {
+            lines.insert(i.0, i.1);
+        }
 
-    #[test]
-    #[should_panic]
-    fn test_parser_jacoco_xml_full_report_with_non_top_level_classes_panics() {
-        let f = File::open("./test/jacoco/full-junit4-report-multiple-top-level-classes.xml")
-            .expect("Failed to open xml file");
+        let mut functions: FunctionMap = FxHashMap::default();
+        for (name, start, executed) in vec![
+            ("Breakpoint#getEntries", 112, false),
+            ("BreakpointValue#xxsmall", 49, false),
+            ("Breakpoint#<clinit>", 104, false),
+            ("BreakpointValue#xxlarge", 97, false),
+            ("BreakpointValue#none", 41, false),
+            ("BreakpointValue#xlarge", 89, false),
+            ("BreakpointValue#setValue", 26, false),
+            ("BreakpointValue#<init>", 5, false),
+            ("BreakpointValue#getBreakpointValueMap", 14, false),
+            ("BreakpointDirection#getEntries", 120, false),
+            ("BreakpointValue#small", 65, false),
+            ("BreakpointDirection#<clinit>", 118, false),
+            ("BreakpointValue#xsmall", 57, false),
+            ("BreakpointValue#large", 81, false),
+            ("BreakpointValue#medium", 73, false),
+        ] {
+            functions.insert(String::from(name), Function { start, executed });
+        }
+
+        let mut branches: BTreeMap<u32, Vec<bool>> = BTreeMap::new();
+        branches.insert(26, vec![false, false, false, false]);
+
+        let expected = vec![(
+            String::from("BreakpointValue.kt"),
+            CovResult {
+                lines,
+                branches,
+                functions,
+            },
+        )];
+
+        let f =
+            File::open("./test/jacoco/kotlin-jacoco-report.xml").expect("Failed to open xml file");
         let file = BufReader::new(&f);
-        let _results = parse_jacoco_xml_report(file).unwrap();
+        let results = parse_jacoco_xml_report(file).unwrap();
+        assert_eq!(results, expected);
     }
 }

src/path_rewriting.rs

@@ -1,4 +1,5 @@
 use globset::{Glob, GlobSet, GlobSetBuilder};
+use log::error;
 use rayon::prelude::*;
 use rustc_hash::FxHashMap;
 use serde_json::Value;
@@ -186,14 +187,14 @@ fn map_partial_path(file_to_paths: &FxHashMap<String, Vec<PathBuf>>, path: PathB
     let mut result: Option<&PathBuf> = None;
     for option in options {
         if option.ends_with(&path) {
-            assert!(
-                result.is_none(),
-                "Only one file in the repository should end with {} ({} and {} both end with that)",
+            if result.is_some() {
+                error!("Only one file in the repository should end with {} ({} and {} both end with that).",
                 path.display(),
                 result.unwrap().display(),
-                option.display()
-            );
-            result = Some(option)
+                option.display());
+            } else {
+                result = Some(option)
+            }
         }
     }
 
@@ -226,7 +227,7 @@ fn to_globset(dirs: &[impl AsRef<str>]) -> GlobSet {
     glob_builder.build().unwrap()
 }
 
-const PARTIAL_PATH_EXTENSION: &str = "java";
+const PARTIAL_PATH_EXTENSION: &[&str] = &["java", "kt"];
 
 pub fn rewrite_paths(
     result_map: CovResultMap,
@@ -258,9 +259,11 @@ pub fn rewrite_paths(
         // The function's purpose is to figure out whether a covered path is inside a subdirectory.
 
         // This only happens for Java, so if there are no Java files, we don't need to do it.
-        let has_java = result_map
-            .keys()
-            .any(|path| check_extension(Path::new(&path), PARTIAL_PATH_EXTENSION));
+        let has_java = result_map.keys().any(|path| {
+            PARTIAL_PATH_EXTENSION
+                .iter()
+                .any(|&ext| check_extension(Path::new(&path), ext))
+        });
 
         // If all covered paths are direct childs of the source directory, then that function will
         // do nothing, and we don't have to do its pre-requisite data gathering.
@@ -296,7 +299,10 @@ pub fn rewrite_paths(
                     panic!("Failed to open directory '{}'.", source_dir.display())
                 });
 
-                if !check_extension(entry.path(), PARTIAL_PATH_EXTENSION) {
+                if !PARTIAL_PATH_EXTENSION
+                    .iter()
+                    .any(|&ext| check_extension(entry.path(), ext))
+                {
                     continue;
                 }
 
@@ -336,12 +342,15 @@ pub fn rewrite_paths(
             let rel_path = remove_prefix(prefix_dir, rel_path);
 
             // Try mapping a partial path to a full path.
-            let rel_path =
-                if map_partial_path_needed && check_extension(&rel_path, PARTIAL_PATH_EXTENSION) {
-                    map_partial_path(&file_to_paths, rel_path)
-                } else {
-                    rel_path
-                };
+            let rel_path = if map_partial_path_needed
+                && PARTIAL_PATH_EXTENSION
+                    .iter()
+                    .any(|&ext| check_extension(&rel_path, ext))
+            {
+                map_partial_path(&file_to_paths, rel_path)
+            } else {
+                rel_path
+            };
 
             // Get absolute path to the source file.
             let (abs_path, rel_path) = get_abs_path(source_dir, rel_path)?;
@@ -1121,6 +1130,33 @@ mod tests {
         assert_eq!(result, empty_result!());
     }
 
+    #[cfg(unix)]
+    #[test]
+    fn test_rewrite_paths_rewrite_path_for_java_kotlin_and_rust() {
+        let mut result_map: CovResultMap = FxHashMap::default();
+        result_map.insert("kt/main.kt".to_string(), empty_result!());
+        result_map.insert("main.rs".to_string(), empty_result!());
+
+        let mut results = rewrite_paths(
+            result_map,
+            None,
+            Some(&canonicalize_path(".").unwrap()),
+            None,
+            true,
+            &[""; 0],
+            &[""; 0],
+            None,
+            Default::default(),
+        );
+        assert!(results.len() == 1);
+
+        let (abs_path, rel_path, result) = results.remove(0);
+        assert!(abs_path.is_absolute());
+        assert!(abs_path.ends_with("test/kotlin/main.kt"));
+        assert_eq!(rel_path, PathBuf::from("test/kotlin/main.kt"));
+        assert_eq!(result, empty_result!());
+    }
+
     #[cfg(windows)]
     #[test]
     fn test_rewrite_paths_rewrite_path_for_java_and_rust() {

src/producer.rs

@@ -776,6 +776,12 @@ mod tests {
                 "jacoco/full-junit4-report-multiple-top-level-classes.xml",
                 false,
             ),
+            (
+                ItemFormat::JacocoXml,
+                false,
+                "jacoco/kotlin-jacoco-report.xml",
+                false,
+            ),
             (ItemFormat::Profraw, true, "default_1.profraw", false),
             (
                 ItemFormat::Gcno,