improve binary artifact and license tests to match other checks

[spencerschrock]

Both Binary-Artifacts and License checks have TODOs about using a gomock repoclient.

scorecard/checks/binary_artifact_test.go

Lines 63 to 65 in 0b9dfb6

	// TODO: Use gMock instead of Localdir here.
	ctrl := gomock.NewController(t)
	repo, err := localdir.MakeLocalDirRepo(tt.inputFolder)

scorecard/checks/license_test.go

Lines 71 to 73 in 0b9dfb6

	// TODO: Use gMock instead of Localdir here.
	ctrl := gomock.NewController(t)
	repo, err := localdir.MakeLocalDirRepo(tt.inputFolder)

Additionally Binary-Artifacts has is a very basic test which just check the score and not other aspects of the CheckResult (like logging).

scorecard/checks/binary_artifact_test.go

Lines 86 to 89 in 0b9dfb6

	result := BinaryArtifacts(&req)
	if result.Score != tt.expected.Score {
	t.Errorf("BinaryArtifacts: %v, expected %v for tests %v", result.Score, tt.expected.Score, tt.name)
	}

It'd be nice to get both of these cleaned up like some of the other checks to:

1. Use gomock
2. use scut.ValidateTestReturn

For example like dangerous_workflow_test.go

scorecard/checks/dangerous_workflow_test.go

Lines 80 to 95 in 0b9dfb6

	dl := scut.TestDetailLogger{}
	ctrl := gomock.NewController(t)
	mockRepoClient := mockrepo.NewMockRepoClient(ctrl)
	mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return(tt.workflowPaths, nil)
	mockRepoClient.EXPECT().GetFileReader(gomock.Any()).DoAndReturn(func(file string) (io.ReadCloser, error) {
	return os.Open("./testdata/" + file)
	}).AnyTimes()
	req := &checker.CheckRequest{
	Ctx: context.Background(),
	RepoClient: mockRepoClient,
	Dlogger: &dl,
	}
	result := DangerousWorkflow(req)
	scut.ValidateTestReturn(t, tt.name, &tt.expected, &result, &dl)

[seelder]

I think maybe I can work on this one? I haven't used gomock before, but it looks like BinaryArtifacts uses a lot of the same functions as DangerousWorkflow so hopefully I can figure it out.

[spencerschrock]

DangerousWorkflow should be a good template, but happy to answer any questions as needed

[seelder]

(@spencerschrock) - currently the ListLicenses function for the localDirRepo client throws an Unsupported Feature error, which is then handled by the raw license check. Options I see include

1. create a mock that just throws an UnsupportedFeature error, similar to the existing functionality (I already have a working version of this, I think)

2. create a mock that actually returns something from ListLicenses, however, that would replicate a lot of the existing functionality for handling these cases in the check itself

3. Testing both 1 and 2 using different test cases with different mocks (adding a couple test cases to do so), although it seems like the difference in functionality should be tested in the raw/checks/license_test rather than here, so this might involve some other test design decisions

My questions are along the lines of - Is the functionality in the license check that handles an unsupportedFeature for listLicenses intended to be permanent? Are any of the options above preferable given the test designs for the project?

I am probably over-thinking this. If so and if option 1 is fine, I can go ahead and submit a PR.

Thanks!

[spencerschrock]

Option (1) sounds reasonable and a more direct translation of the existing tests, and would be a good first PR.

Depending on how much you want to do, creating a separate test which uses ListLicenses (effectively option 3) also sounds good.