Skip to content

chore(auth/grpctransport): remove env impact on direct path tests#14186

Open
westarle wants to merge 1 commit intogoogleapis:mainfrom
westarle:fix-directpath-env
Open

chore(auth/grpctransport): remove env impact on direct path tests#14186
westarle wants to merge 1 commit intogoogleapis:mainfrom
westarle:fix-directpath-env

Conversation

@westarle
Copy link

This clears the GOOGLE_CLOUD_DISABLE_DIRECT_PATH environment variable in TestLogDirectPathMisconfig tests to prevent failures when the environment variable is set in the environment running the tests. This fixes a nuisance test error when running tests on cloudtop.

…irectpath tests

This clears the GOOGLE_CLOUD_DISABLE_DIRECT_PATH environment variable in
TestLogDirectPathMisconfig tests to prevent failures when the environment
variable is set in the environment running the tests.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This PR correctly makes the direct path tests independent of the environment by unsetting GOOGLE_CLOUD_DISABLE_DIRECT_PATH. To improve the test suite's maintainability, I've suggested refactoring the three modified tests, which share a lot of boilerplate, into a single table-driven test. This would reduce code duplication and make future modifications easier.

}

func TestLogDirectPathMisconfigDirectPathNotSet(t *testing.T) {
t.Setenv(disableDirectPathEnvVar, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line is also added to TestLogDirectPathMisconfigWrongCredential and TestLogDirectPathMisconfigNotOnGCE. Since these three tests are very similar, consider refactoring them into a single table-driven test. This would reduce code duplication and improve maintainability. You would only need to call t.Setenv once in the parent test.

Here's an example of how it could look:

func TestLogDirectPathMisconfig(t *testing.T) {
	t.Setenv(disableDirectPathEnvVar, "")

	testCases := []struct {
		name        string
		opts        *Options
		creds       *google.Credentials
		onGCE       bool
		wantWarning string
	}{
		{
			name: "DirectPathNotSet",
			opts: &Options{InternalOptions: &InternalOptions{
				EnableDirectPathXds: true,
			}},
			wantWarning: directPathNotSetWarning,
		},
		{
			name: "WrongCredential",
			opts: &Options{InternalOptions: &InternalOptions{
				EnableDirectPathXds: true,
				EnableDirectPath:    true,
			}},
			creds: &google.Credentials{
				TokenSource: oauth2.StaticTokenSource(&oauth2.Token{}),
			},
			wantWarning: wrongCredsWarning,
		},
		{
			name: "NotOnGCE",
			opts: &Options{InternalOptions: &InternalOptions{
				EnableDirectPath:    true,
				EnableDirectPathXds: true,
			}},
			creds: &google.Credentials{
				ProjectID: "project-id",
			},
			wantWarning: notOnGCEWarning,
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			var buf bytes.Buffer
			log.SetOutput(&buf)
			defer log.SetOutput(os.Stderr)

			logDirectPathMisconfig(tc.opts, tc.creds, tc.onGCE)

			if got := buf.String(); !strings.Contains(got, tc.wantWarning) {
				t.Errorf("got %q, want substring %q", got, tc.wantWarning)
			}
		})
	}
}

You would then remove TestLogDirectPathMisconfigDirectPathNotSet, TestLogDirectPathMisconfigWrongCredential, and TestLogDirectPathMisconfigNotOnGCE.

@westarle westarle changed the title test(auth/grpctransport): remove env impact on direct path tests chore(auth/grpctransport): remove env impact on direct path tests Mar 16, 2026
@westarle westarle marked this pull request as ready for review March 16, 2026 23:24
@westarle westarle requested review from a team as code owners March 16, 2026 23:24
@westarle westarle enabled auto-merge (squash) March 16, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant