Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Erroneous inconclusive CI-Tests result #96

Closed
moorereason opened this issue Dec 20, 2020 · 2 comments · Fixed by #108
Closed

Erroneous inconclusive CI-Tests result #96

moorereason opened this issue Dec 20, 2020 · 2 comments · Fixed by #108

Comments

@moorereason
Copy link
Contributor

Given:

scorecard/checks/ci_tests.go

Lines 58 to 59 in 1991617

if totalTested == 0 {
return checker.InconclusiveResult

Shouldn't that be if totalMerged == 0?

If we have 10 merged PRs and zero tests found for those PRs, that should not be an inconclusive result. It should Fail 10 not Fail 0. Or am I missing something?
@moorereason
Copy link
Contributor Author

moorereason commented Dec 20, 2020 •
edited
Loading

I think I see what's happening here. GithubStatuses returns an inconclusive result if no tests passed because there may be passing results in GithubCheckRuns. Hmm. The MultiCheck makes resolving #95 more complicated.

@dlorenc
Copy link
Contributor

dlorenc commented Dec 20, 2020

Yeah, this is a tough case. It's hard to definitively prove a negative here, so I erred on the side of caution. I don't feel very strongly here either way, if you think something else makes more sense that's fine with me!

moorereason added a commit to moorereason/scorecard that referenced this issue Dec 23, 2020
@moorereason
Refactor CI-Tests to show negative results
39464a5 
Add negative check results to the CI-Tests output.

Assuming that a repo will only support one CI system, GithubStatuses and
GithubCheckRuns are merged into a single CITests function.  Since both
GithubStatuses and GithubCheckRuns were essentially validating the same
PRs, it makes more sense to keep all of that state together in a single
check.

Additionaly, a single check can reduce the number of API queries once we
detect the CI system in use.

Fixes ossf#96
Updates ossf#95
@moorereason moorereason mentioned this issue Dec 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor CI-Tests to show negative results moorereason/scorecard
2 participants
@moorereason @dlorenc