mozilla / grcov

Rust tool to collect and aggregate code coverage data for multiple source files
Mozilla Public License 2.0
1.19k stars 149 forks source link

Coveralls's API requires "service_job_id" field #373

Closed redtankd closed 4 years ago

redtankd commented 4 years ago

According Coveralls' API Document, service_job_id is used to reference a repository.

We can reference a repository in one of two ways:

service_job_id String and service_name String

Coveralls currently supports the following continuous integration services: Travis CI (open source), Travis Pro (private repos), CircleCI, Semaphore, JenkinsCI, or Codeship.

Setting service_name to "travis-ci" and service_job_id to the Travis Job ID will automatically look up the appropriate build and repository, and assign the job correctly on Coveralls.

But grcov generates service-job-number now. Although Coveralls can accept grcov's output, there are some problems. For example the link from Coveralls to Travis doesn't work.

Minoru commented 4 years ago

Wow, good catch @redtankd! I wonder how I overlooked this while working on #361. I'm checking right now what will happen if I simply replace "service_job_number" key with "service_job_id".

Minoru commented 4 years ago

I don't see any change in the results. I'd keep the change regardless, because it's in line with Coveralls docs—but this decision is up to @marco-c.

My changes are here: https://github.com/Minoru/grcov/commit/8c2a4b96ef773ecbb3cca8bf4b26b2f3f9d24c3a (tests weren't updated, but I can take care of them if you want me to submit a PR)

Example Travis build: https://travis-ci.org/newsboat/newsboat/jobs/635698878

Coveralls report: https://coveralls.io/builds/28046645

For comparison:

Minoru commented 4 years ago

Disregard my previous comment: I didn't commit changes to the example project. Please wait for the new results.

Minoru commented 4 years ago

Updated results:

The report is very much improved:

I'm pretty sure this change will be accepted, so I'm going to submit a pull request once I fixed the tests. I now wonder how important are the changes I made in #366, though. Need to test what happens if I don't pass --commit-sha to grcov.

Minoru commented 4 years ago

Results without --commit-sha look practically the same:

@marco-c, should --commit-sha simply be dropped?

marco-c commented 4 years ago

@Minoru thanks for testing! I wouldn't drop it, the additional data might be useful for other services (e.g. codecov) or people relying on this output format from grcov to generate their own reports.

Minoru commented 4 years ago

Then at least it should be optional (--commit-sha is currently required for "coveralls" and "coveralls+" reports). PRs forthcoming.

redtankd commented 4 years ago

Great job! Thank you very much! @Minoru

Minoru commented 4 years ago

One more reason to fix this: on Travis, Coveralls' token is usually put into an encrypted variable, to hide it from logs—but it also hides the token from PRs. As a result, only PRs created from within the repository can submit coverage to Coveralls. That's exactly how I was testing grcov so far, so I never noticed the problem; but the newest "outside" PR to my project quickly exposed the problem (cf. https://github.com/newsboat/newsboat/issues/721). If grcov were to use the correct key, Coveralls would use that for authentication, and PRs would be able to submit coverage even without knowing the token,

Minoru commented 4 years ago

Addendum to my previous comment: for that idea to work, we'll have to make --token optional, requiring either --token or a combination of --service-name and --service-job-id. Right now, --token is required, and if I do something like --token ${COVERALLS_REPO_TOKEN}, the JSON contains an empty string and makes Coveralls reject the submission even though it could authenticate the submission via service-name and job-id.

This is addressed in https://github.com/mozilla/grcov/pull/381.