BUG: re-enable some e2e tests

[laurentsimon]

We recently introduced #860 because all the e2e tests are disabled. I think we can safely re-enable those for which we have create a repo - see https://github.com/ossf-tests/
I created scorecard-check-pinned-dependencies-e2e, scorecard-check-token-permissions-e2e, scorecard-check-branch-protection-e2e and scorecard-check-binary-artifacts-e2e. Those are safe to be re-enabled.

@azeemsgoogle how about scorecard-check-packaging-e2e?
@oliverchang how about scorecard-check-vulnerabilities-open62541?

[laurentsimon]

@naveensrinivasan you have the most knowledge about the e2e tests integration. Do you think you can re-enable the checks for the e2e2 listed above?

[nanikjava]

@laurentsimon tested make e2e from main and the following tests failed:

Summarizing 4 Failures:

[Fail] E2E TEST:Packaging E2E TEST:Validating use of packaging in CI/CD [It] Should return use of packaging in CI/CD 
/home/nanik/GolandProjects/gopath/src/github.com/ossf/scorecard/e2e/packaging_test.go:59

[Fail] E2E TEST:Branch-Protection E2E TEST:Validating branch protection E2E TEST:Validating branch protection [It] Should fail to return branch protection on other repositories 
/home/nanik/GolandProjects/gopath/src/github.com/ossf/scorecard/e2e/branch_protection_test.go:81

[Fail] E2E TEST:Branch-Protection E2E TEST:Validating branch protection [It] Should fail to return branch protection on other repositories 
/home/nanik/GolandProjects/gopath/src/github.com/ossf/scorecard/e2e/branch_protection_test.go:52

[Fail] E2E TEST:CI-Tests E2E TEST:Validating use of CI tests [It] Should return use of CI tests 
/home/nanik/GolandProjects/gopath/src/github.com/ossf/scorecard/e2e/ci_tests_test.go:60

Looked at the test code for E2E TEST:Packaging and it is failing because the following action for publish-snapshot in this link https://github.com/ossf-tests/scorecard-check-packaging-e2e/actions/runs/1089006870 have got skipped actions there isn't any success action. Once there is some success action the test will pass

Changing the Status from success to skipped in the following code

		runs, _, err := c.Client.Actions.ListWorkflowRunsByFileName(c.Ctx, c.Owner, c.Repo, filepath.Base(fp),
			&github.ListWorkflowRunsOptions{
				Status: "skipped",
			})

pass the test.

[laurentsimon]

Thanks. I'll bring this up during the meeting today.

• packaging: @azeemsgoogle, please comment
• branch protection: there's been a recent commit cc312f2 which I suspect makes the tests fail now. We need to remove apache/airflow. I think we'll need to either fix ossf-tests/scorecard-check-branch-protection-e2e or leave it out of the e2e entirely unless we have admin tokens to query the repo. For now, it's simpler to leave it out.
• ci-test: this still relies on apache/airflow. We need a new repo. I've created https://github.com/ossf-tests/scorecard-check-ci-tests-e2e. Can you send a PR thats add some files to set up the repo?

[azeemshaikh38]

Yes, packaging test is safe to be enabled.

[laurentsimon]

Yes, packaging test is safe to be enabled.

any comment on Status from success to skipped that @nanikjava suggested? Is that intended or should we fix the test repo so that it contains a success run?

[azeemshaikh38]

Ah I see. So I copied over the .github folder from apache/orc (the previous repo we were testing) and kept the test and everything else same - #796. I guess, not having files in the fake repo causes the workflow runs to be skipped. We need to update the fake repo such that there are successful workflow runs. We can copy over the entire apache/orc repo and see if that helps.

[naveensrinivasan]

@laurentsimon tested make e2e from main and the following tests failed:

Summarizing 4 Failures:

[Fail] E2E TEST:Packaging E2E TEST:Validating use of packaging in CI/CD [It] Should return use of packaging in CI/CD 
/home/nanik/GolandProjects/gopath/src/github.com/ossf/scorecard/e2e/packaging_test.go:59

[Fail] E2E TEST:Branch-Protection E2E TEST:Validating branch protection E2E TEST:Validating branch protection [It] Should fail to return branch protection on other repositories 
/home/nanik/GolandProjects/gopath/src/github.com/ossf/scorecard/e2e/branch_protection_test.go:81

[Fail] E2E TEST:Branch-Protection E2E TEST:Validating branch protection [It] Should fail to return branch protection on other repositories 
/home/nanik/GolandProjects/gopath/src/github.com/ossf/scorecard/e2e/branch_protection_test.go:52

[Fail] E2E TEST:CI-Tests E2E TEST:Validating use of CI tests [It] Should return use of CI tests 
/home/nanik/GolandProjects/gopath/src/github.com/ossf/scorecard/e2e/ci_tests_test.go:60

Looked at the test code for E2E TEST:Packaging and it is failing because the following action for publish-snapshot in this link https://github.com/ossf-tests/scorecard-check-packaging-e2e/actions/runs/1089006870 have got skipped actions there isn't any success action. Once there is some success action the test will pass

Changing the Status from success to skipped in the following code

		runs, _, err := c.Client.Actions.ListWorkflowRunsByFileName(c.Ctx, c.Owner, c.Repo, filepath.Base(fp),
			&github.ListWorkflowRunsOptions{
				Status: "skipped",
			})

pass the test.

@nanikjava Are you planning to work on this? Please let us know. Thanks

[nanikjava]

@naveensrinivasan No plan currently to work on it.

[azeemshaikh38]

Thanks @nanikjava ! @naveensrinivasan assigning to you.

[azeemshaikh38]

Increasing the scope of this bug to include #1113 (comment)

[azeemshaikh38]

Re-opening. We still need the e2e for generic RepoClient interface.

[azeemshaikh38]

Breaking e2e tests again - #1253 (comment)

How do we fix this? Why does GitHub allow us to merge PRs which break e2e tests even though integration tests are required for a PR to be merged? @laurentsimon @naveensrinivasan @oliverchang any ideas?

[laurentsimon]

this says the check was skipped https://github.com/ossf/scorecard/runs/4219890078?check_suite_focus=true

[azeemshaikh38]

Ok, I think I just found a clue. Looks like PRs which come from a fork of Scorecard always skip the integration tests. But PRs which are not from a fork, the e2e tests are run. Most likely this line is the culprit:

scorecard/.github/workflows/integration.yml

Line 33 in 4502dfb

if: (github.event_name == 'repository_dispatch') || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository)

.

Specifically, the part which says github.event.pull_request.head.repo.full_name == github.repository. I'll try and look further but if someone understands this better feel free to take over.

[laurentsimon]

I think you're on the right track: all my PRs don't trigger a run and I use a fork, see https://github.com/ossf/scorecard/actions/workflows/integration.yml

[laurentsimon]

I think the original intention was to use the ok-to-test because there's a bunch of secrets in this workflow, and pull_request don't have access to them.
What's special about secret secrets.GH_AUTH_TOKEN? Does it have admin privilege because it's used to test the branch protection check? or can we use the less-privileged default GITHUB_TOKEN the job receives automatically from GitHub?

I think we can separate this part of the workflow and use the pull_request: it will use a non-admin token which would be fine? For admin use, how about running the test on push event to main instead? Or can we make a workflow run only if it's been LGTM'ed? There is still a risk someone changes the PR after LGTM... but in this case they could also just sneak in some bad code in scorecard which will run on all clients/GH actions.

[azeemshaikh38]

More breaking tests :( @laurentsimon could you look into it, I think the failures started after #1244 and #1252. Maybe run make e2e locally before submitting while we look to resolve this issue.

[azeemshaikh38]

I think the original intention was to use the ok-to-test because there's a bunch of secrets in this workflow, and pull_request don't have access to them. What's special about secret secrets.GH_AUTH_TOKEN? Does it have admin privilege because it's used to test the branch protection check? or can we use the less-privileged default GITHUB_TOKEN the job receives automatically from GitHub?

I think we can separate this part of the workflow and use the pull_request: it will use a non-admin token which would be fine? For admin use, how about running the test on push event to main instead? Or can we make a workflow run only if it's been LGTM'ed? There is still a risk someone changes the PR after LGTM... but in this case they could also just sneak in some bad code in scorecard which will run on all clients/GH actions.

+1. I'm completely in favor of doing away with this if condition and running e2e tests on all PRs. It's repeatedly causing loss of productivity. @naveensrinivasan wdut? Can we remove it?

[naveensrinivasan]

I agree we can try that. Also, remember some of the tests look for GitHub Scorecard data on the main repo. If the PR isn't merged then it is going to run successfully then have an issue next time a new PR comes in. One of the test which will likely fix is the local repo test.