lostluck commented on a change in pull request #16643: URL: https://github.com/apache/beam/pull/16643#discussion_r794962040
########## File path: sdks/go/pkg/beam/options/gcpopts/options_test.go ########## @@ -0,0 +1,79 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gcpopts + +import ( + "fmt" + "io/ioutil" + "os" + "testing" +) + +func TestGetProjectFromFlagOrEnvironmentWithNoProjectSet(t *testing.T) { + setupFakeCredentialFile(t, "") + *Project = "" + projectId := GetProjectFromFlagOrEnvironment(nil) + if projectId != "" { + t.Fatalf("%q returned as project id, should be \"\"", projectId) Review comment: It's best to identify the function, the relevant inputs (here there aren't any, unless one wants to repeat the case information), what we wanted, and what we got. So here, (and below), when possible, you can use a similar format. ``` if got, want := GetProjectFromFlagOrEnvironment(nil), ""; got != want { t.Fatalf("GetProjectFromFlagOrEnvironment() = %q, want %q", got, want) } ``` It clearly indicates the call being run, and avoids repeating what we want and expect, and follows the "got, want" convention (eliding the got, since it's clear from the presentation that it's the output of the function). I have a personal preference to lean on defining both got and want within the if scope, so I don't make copy paste errors, when making various checks. See https://github.com/golang/go/wiki/TestComments#got-before-want and the following sections. Overall it's a pretty good doc. ########## File path: sdks/go/pkg/beam/options/gcpopts/options.go ########## @@ -37,9 +39,22 @@ var ( // GetProject returns the project, if non empty and exits otherwise. // Convenience function. func GetProject(ctx context.Context) string { - if *Project == "" { + project := GetProjectFromFlagOrEnvironment(ctx) Review comment: While we can't meaningfully test the "exit" path, can we consider adding a test for the GetProject happy path (where the project is set)? That will probably satisfy the code coverage diff, and overall increase coverage on this file. ########## File path: sdks/go/pkg/beam/options/gcpopts/options_test.go ########## @@ -0,0 +1,79 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gcpopts + +import ( + "fmt" + "io/ioutil" + "os" + "testing" +) + +func TestGetProjectFromFlagOrEnvironmentWithNoProjectSet(t *testing.T) { Review comment: The naming convention for tests we've been doing is `Test<Function/Struct>_<method/case>(t *testing.T) { ... }` Even in the test tutorial: https://go.dev/doc/tutorial/add-a-test , you'll note that we don't use "With" to denote the case if at all. But it's also not that beneficial to specify what's being set. In these tests, it's unambiguously going to be the project, so the conditions are on the Flag, or the Environment. If any of these fail, the shorter test names with that _ make them easier to read in the test output. Sp please rename the tests to follow the convention: For this one, it would be `TestGetProjectFromFlagOrEnvironment_Unset` Similarly down below: FlagSet, EnvironmentSet, BothSet ########## File path: sdks/go/pkg/beam/options/gcpopts/options_test.go ########## @@ -0,0 +1,79 @@ +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to You under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package gcpopts + +import ( + "fmt" + "io/ioutil" + "os" + "testing" +) + +func TestGetProjectFromFlagOrEnvironmentWithNoProjectSet(t *testing.T) { + setupFakeCredentialFile(t, "") + *Project = "" + projectId := GetProjectFromFlagOrEnvironment(nil) + if projectId != "" { + t.Fatalf("%q returned as project id, should be \"\"", projectId) + } +} + +func TestGetProjectFromFlagOrEnvironmentWithProjectFlagSet(t *testing.T) { + setupFakeCredentialFile(t, "") + *Project = "test" + projectId := GetProjectFromFlagOrEnvironment(nil) + if projectId != "test" { + t.Fatalf("%q returned as project id, should be \"test\"", projectId) + } +} + +func TestGetProjectFromFlagOrEnvironmentWithNoProjectFlagSetAndFallbackSet(t *testing.T) { + setupFakeCredentialFile(t, "fallback") + *Project = "" + projectId := GetProjectFromFlagOrEnvironment(nil) + if projectId != "fallback" { + t.Fatalf("%q returned as project id, should be \"fallback\"", projectId) + } +} + +func TestGetProjectFromFlagOrEnvironmentWithProjectFlagSetAndFallbackSet(t *testing.T) { + setupFakeCredentialFile(t, "fallback") + *Project = "test" + projectId := GetProjectFromFlagOrEnvironment(nil) + if projectId == "fallback" { + t.Fatalf("fallback returned as project id, should have used the flag setting of \"test\"") + } else if projectId != "test" { + t.Fatalf("%q returned as project id, should be \"test\"", projectId) + } +} + +// Set up fake credential file to read project from with the passed in projectId. +func setupFakeCredentialFile(t *testing.T, projectId string) { Review comment: Since this is a test helper function, t.Helper should be called at the top. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
