Coveralls: make repo token optional (#381)

* Make --service-job-number an alias of service-job-id

Implementing this in Clap simplifies the code that uses the values, and
also produces a better --help message.

* For Coveralls, require either token or (name, job_id)

* Coveralls: omit token/(service_name, job_id) from output if absent on CLI

If we don't pass --token to grcov, the JSON contains an empty string
(`{"token": ""}`), which Coveralls tries to use and fails.

README.md

@@ -54,8 +54,7 @@ OPTIONS:
     -p, --prefix-dir <PATH>
             Specifies a prefix to remove from the paths (e.g. if grcov is run on a different machine than the one that
             generated the code coverage information)
-        --service-job-id <SERVICE JOB ID>            Sets the service job id
-        --service-job-number <SERVICE JOB NUMBER>    Sets the service job number (deprecated in favour of --service-job-id)
+        --service-job-id <SERVICE JOB ID>            Sets the service job id [aliases: service-job-number]
         --service-name <SERVICE NAME>                Sets the service name
         --service-number <SERVICE NUMBER>            Sets the service number
         --service-pull-request <SERVICE PULL REQUEST>

src/main.rs

@@ -7,7 +7,7 @@ extern crate serde_json;
 extern crate simplelog;
 extern crate tempfile;
 
-use clap::{crate_authors, crate_version, App, Arg};
+use clap::{crate_authors, crate_version, App, Arg, ArgGroup};
 use crossbeam::crossbeam_channel::unbounded;
 use log::error;
 use rustc_hash::FxHashMap;
@@ -51,7 +51,11 @@ fn main() {
                                .value_name("OUTPUT TYPE")
                                .default_value("lcov")
                                .possible_values(&["ade", "lcov", "coveralls", "coveralls+", "files", "covdir", "html"])
-                               .takes_value(true))
+                               .takes_value(true)
+                               .requires_ifs(&[
+                                   ("coveralls", "coveralls_auth"),
+                                   ("coveralls+", "coveralls_auth")
+                               ]))
 
                           .arg(Arg::with_name("output_file")
                                .help("Specifies the output file")
@@ -111,11 +115,7 @@ fn main() {
                                .help("Sets the repository token from Coveralls, required for the 'coveralls' and 'coveralls+' formats")
                                .long("token")
                                .value_name("TOKEN")
-                               .takes_value(true)
-                               .required_ifs(&[
-                                   ("output_type", "coveralls"),
-                                   ("output_type", "coveralls+")
-                               ]))
+                               .takes_value(true))
 
                           .arg(Arg::with_name("commit_sha")
                                .help("Sets the hash of the commit used to generate the code coverage data")
@@ -135,19 +135,13 @@ fn main() {
                                .value_name("SERVICE NUMBER")
                                .takes_value(true))
 
-                          .arg(Arg::with_name("service_job_number")
-                               .help("Sets the service job number (deprecated in favour of --service-job-id)")
-                               .long("service-job-number")
-                               .value_name("SERVICE JOB NUMBER")
-                               .takes_value(true)
-                               .conflicts_with("service_job_id"))
-
                           .arg(Arg::with_name("service_job_id")
                                .help("Sets the service job id")
                                .long("service-job-id")
                                .value_name("SERVICE JOB ID")
                                .takes_value(true)
-                               .conflicts_with("service_job_number"))
+                               .visible_alias("service-job-number")
+                               .requires("service_name"))
 
                           .arg(Arg::with_name("service_pull_request")
                                .help("Sets the service pull request number")
@@ -182,6 +176,14 @@ fn main() {
                                .value_name("LOG")
                                .takes_value(true))
 
+                          // This group requires that at least one of --token and --service-job-id
+                          // be present. --service-job-id requires --service-name, so this
+                          // effectively means we accept the following combinations:
+                          // - --token
+                          // - --token --service-job-id --service-name
+                          // - --service-job-id --service-name
+                          .group(ArgGroup::with_name("coveralls_auth").args(&["token", "service_job_id"]).multiple(true))
+
                           .get_matches();
 
     let paths: Vec<_> = matches.values_of("paths").unwrap().collect();
@@ -208,15 +210,12 @@ fn main() {
         None
     };
     let is_llvm = matches.is_present("llvm");
-    let repo_token = matches.value_of("token").unwrap_or("");
+    let repo_token = matches.value_of("token");
     let commit_sha = matches.value_of("commit_sha").unwrap_or("");
-    let service_name = matches.value_of("service_name").unwrap_or("");
+    let service_name = matches.value_of("service_name");
     let is_parallel = matches.is_present("parallel");
     let service_number = matches.value_of("service_number").unwrap_or("");
-    let service_job_id = matches
-        .value_of("service_job_id")
-        .or_else(|| matches.value_of("service_job_number"))
-        .unwrap_or("");
+    let service_job_id = matches.value_of("service_job_id");
     let service_pull_request = matches.value_of("service_pull_request").unwrap_or("");
     let vcs_branch = matches.value_of("vcs_branch").unwrap_or("");
     let log = matches.value_of("log").unwrap_or("");

src/output.rs

@@ -366,10 +366,10 @@ fn get_coveralls_git_info(commit_sha: &str, vcs_branch: &str) -> Value {
 
 pub fn output_coveralls(
     results: CovResultIter,
-    repo_token: &str,
-    service_name: &str,
+    repo_token: Option<&str>,
+    service_name: Option<&str>,
     service_number: &str,
-    service_job_id: &str,
+    service_job_id: Option<&str>,
     service_pull_request: &str,
     commit_sha: &str,
     with_function_info: bool,
@@ -431,21 +431,28 @@ pub fn output_coveralls(
 
     let git = get_coveralls_git_info(commit_sha, vcs_branch);
 
+    let mut result = json!({
+        "git": git,
+        "source_files": source_files,
+        "service_number": service_number,
+        "service_pull_request": service_pull_request,
+        "parallel": parallel,
+    });
+
+    if let (Some(repo_token), Some(obj)) = (repo_token, result.as_object_mut()) {
+        obj.insert("repo_token".to_string(), json!(repo_token));
+    }
+
+    if let (Some(service_name), Some(obj)) = (service_name, result.as_object_mut()) {
+        obj.insert("service_name".to_string(), json!(service_name));
+    }
+
+    if let (Some(service_job_id), Some(obj)) = (service_job_id, result.as_object_mut()) {
+        obj.insert("service_job_id".to_string(), json!(service_job_id));
+    }
+
     let mut writer = BufWriter::new(get_target_output_writable(output_file));
-    serde_json::to_writer(
-        &mut writer,
-        &json!({
-            "repo_token": repo_token,
-            "git": git,
-            "source_files": source_files,
-            "service_name": service_name,
-            "service_number": service_number,
-            "service_job_id": service_job_id,
-            "service_pull_request": service_pull_request,
-            "parallel": parallel,
-        }),
-    )
-    .unwrap();
+    serde_json::to_writer(&mut writer, &result).unwrap();
 }
 
 pub fn output_files(results: CovResultIter, output_file: Option<&str>) {
@@ -610,10 +617,10 @@ mod tests {
         let parallel: bool = true;
         output_coveralls(
             results,
+            None,
+            None,
             "unused",
-            "unused",
-            "unused",
-            expected_service_job_id,
+            Some(expected_service_job_id),
             "unused",
             "unused",
             with_function_info,
@@ -626,4 +633,84 @@ mod tests {
 
         assert_eq!(results["service_job_id"], expected_service_job_id);
     }
+
+    #[test]
+    fn test_coveralls_token_field_is_absent_if_arg_is_none() {
+        let tmp_dir = tempfile::tempdir().expect("Failed to create temporary directory");
+        let file_name = "test_coveralls_token.json";
+        let file_path = tmp_dir.path().join(&file_name);
+
+        let results = vec![(
+            PathBuf::from("foo/bar/a.cpp"),
+            PathBuf::from("foo/bar/a.cpp"),
+            CovResult {
+                lines: [(1, 10), (2, 11)].iter().cloned().collect(),
+                branches: BTreeMap::new(),
+                functions: FxHashMap::default(),
+            },
+        )];
+
+        let results = Box::new(results.into_iter());
+        let token = None;
+        let with_function_info: bool = true;
+        let parallel: bool = true;
+        output_coveralls(
+            results,
+            token,
+            None,
+            "unused",
+            None,
+            "unused",
+            "unused",
+            with_function_info,
+            Some(file_path.to_str().unwrap()),
+            "unused",
+            parallel,
+        );
+
+        let results: Value = serde_json::from_str(&read_file(&file_path)).unwrap();
+
+        assert_eq!(results.get("repo_token"), None);
+    }
+
+    #[test]
+    fn test_coveralls_service_fields_are_absent_if_args_are_none() {
+        let tmp_dir = tempfile::tempdir().expect("Failed to create temporary directory");
+        let file_name = "test_coveralls_service_fields.json";
+        let file_path = tmp_dir.path().join(&file_name);
+
+        let results = vec![(
+            PathBuf::from("foo/bar/a.cpp"),
+            PathBuf::from("foo/bar/a.cpp"),
+            CovResult {
+                lines: [(1, 10), (2, 11)].iter().cloned().collect(),
+                branches: BTreeMap::new(),
+                functions: FxHashMap::default(),
+            },
+        )];
+
+        let results = Box::new(results.into_iter());
+        let service_name = None;
+        let service_job_id = None;
+        let with_function_info: bool = true;
+        let parallel: bool = true;
+        output_coveralls(
+            results,
+            None,
+            service_name,
+            "unused",
+            service_job_id,
+            "unused",
+            "unused",
+            with_function_info,
+            Some(file_path.to_str().unwrap()),
+            "unused",
+            parallel,
+        );
+
+        let results: Value = serde_json::from_str(&read_file(&file_path)).unwrap();
+
+        assert_eq!(results.get("service_name"), None);
+        assert_eq!(results.get("service_job_id"), None);
+    }
 }

tests/test.rs

@@ -10,7 +10,7 @@ use serde_json::Value;
 use std::fs::File;
 use std::io::{Read, Write};
 use std::path::{Path, PathBuf};
-use std::process::Command;
+use std::process::{Command, Stdio};
 use std::{env, fs};
 use walkdir::WalkDir;
 use zip::write::FileOptions;
@@ -122,31 +122,8 @@ fn read_expected(
     read_file(&path.join(&name))
 }
 
-fn run_grcov(paths: Vec<&Path>, source_root: &Path, output_format: &str) -> String {
-    let mut args: Vec<&str> = Vec::new();
-
-    for path in &paths {
-        args.push(path.to_str().unwrap());
-    }
-    args.push("-t");
-    args.push(output_format);
-    if output_format == "coveralls" {
-        args.push("--token");
-        args.push("TOKEN");
-        args.push("--commit-sha");
-        args.push("COMMIT");
-        args.push("-s");
-        args.push(source_root.to_str().unwrap());
-        args.push("--branch");
-    }
-    args.push("--ignore");
-    args.push("C:/*");
-    args.push("--ignore");
-    args.push("/usr/*");
-    args.push("--ignore");
-    args.push("/Applications/*");
-    args.push("--guess-directory-when-missing");
-
+/// Returns the path to grcov executable.
+fn get_cmd_path() -> &'static str {
     let mut cmd_path = if cfg!(windows) {
         ".\\target\\debug\\grcov.exe"
     } else {
@@ -161,7 +138,39 @@ fn run_grcov(paths: Vec<&Path>, source_root: &Path, output_format: &str) -> Stri
         };
     }
 
-    let output = Command::new(cmd_path)
+    cmd_path
+}
+
+fn run_grcov(paths: Vec<&Path>, source_root: &Path, output_format: &str) -> String {
+    let mut args: Vec<&str> = Vec::new();
+
+    for path in &paths {
+        args.push(path.to_str().unwrap());
+    }
+    args.push("-t");
+    args.push(output_format);
+    if output_format == "coveralls" {
+        args.push("--token");
+        args.push("TOKEN");
+        args.push("--service-name");
+        args.push("");
+        args.push("--service-job-id");
+        args.push("");
+        args.push("--commit-sha");
+        args.push("COMMIT");
+        args.push("-s");
+        args.push(source_root.to_str().unwrap());
+        args.push("--branch");
+    }
+    args.push("--ignore");
+    args.push("C:/*");
+    args.push("--ignore");
+    args.push("/usr/*");
+    args.push("--ignore");
+    args.push("/Applications/*");
+    args.push("--guess-directory-when-missing");
+
+    let output = Command::new(get_cmd_path())
         .args(args)
         .output()
         .expect("Failed to run grcov");
@@ -694,3 +703,63 @@ fn test_integration_guess_single_file() {
         &run_grcov(vec![&zip_path], &PathBuf::from(""), "covdir"),
     );
 }
+
+#[test]
+fn test_coveralls_works_with_just_token_arg() {
+    for output in &["coveralls", "coveralls+"] {
+        let status = Command::new(get_cmd_path())
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .args(vec![".", "-t", output, "--token", "123"])
+            .status()
+            .expect("Failed to run grcov");
+        assert!(status.success());
+    }
+}
+
+#[test]
+fn test_coveralls_works_with_just_service_name_and_job_id_args() {
+    for output in &["coveralls", "coveralls+"] {
+        let status = Command::new(get_cmd_path())
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .args(vec![
+                ".",
+                "-t",
+                output,
+                "--service-name",
+                "travis-ci",
+                "--service-job-id",
+                "456",
+            ])
+            .status()
+            .expect("Failed to run grcov");
+        assert!(status.success());
+    }
+}
+
+#[test]
+fn test_coveralls_service_name_is_not_sufficient() {
+    for output in &["coveralls", "coveralls+"] {
+        let status = Command::new(get_cmd_path())
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .args(vec![".", "-t", output, "--service-name", "travis-ci"])
+            .status()
+            .expect("Failed to run grcov");
+        assert!(!status.success());
+    }
+}
+
+#[test]
+fn test_coveralls_service_job_id_is_not_sufficient() {
+    for output in &["coveralls", "coveralls+"] {
+        let status = Command::new(get_cmd_path())
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .args(vec![".", "-t", output, "--service-job-id", "456"])
+            .status()
+            .expect("Failed to run grcov");
+        assert!(!status.success());
+    }
+}