Add additional test coverage for helper functions in Util #4651

gajananan · 2024-10-05T10:46:17Z

Summary

Currently helper functions in Util lacks test coverage

This commit adds test coverage helper functions in Util

Ref #4380

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

Currently helper functions in Util lacks test coverage This commit adds test coverage helper functions in Util Signed-off-by: Kugamoorthy Gajananan <[email protected]>

coveralls · 2024-10-05T10:57:01Z

coverage: 53.769% (+0.5%) from 53.264%
when pulling 00dd4ba on gajananan:improve-test-coverage-util-helpers
into 49f192a on stacklok:main.

Currently helper functions in Util lacks test coverage This commit fixes the lint errors and handles errors. Signed-off-by: Kugamoorthy Gajananan <[email protected]>

Currently helper functions in Util lacks test coverage This commit - fixes the lint errors and handles errors. - adds test coverage for OpenFileArg, ExpandFileArgs, functions Signed-off-by: Kugamoorthy Gajananan <[email protected]>

Currently helper functions in Util lacks test coverage. This commit - fixes the lint errors and handles errors. - Adds test coverage for LoadCredentials function Signed-off-by: Kugamoorthy Gajananan <[email protected]>

Currently helper functions in Util lacks test coverage This commit fixes the lint errors after fixing lint version. Signed-off-by: Kugamoorthy Gajananan <[email protected]>

Currently helper functions in Util lacks test coverage This commit adds test coverage for RevokeToken function. Signed-off-by: Kugamoorthy Gajananan <[email protected]>

JAORMX · 2024-10-08T12:56:55Z

internal/util/helpers_test.go

+			t.Parallel()
+
+			// Create a unique directory for each test
+			testDir := fmt.Sprintf("testdir_%s", sanitizeTestName(tt.name))


You could probably leverage golang's utilities instead: https://pkg.go.dev/testing#B.TempDir

This will create a temporary directory for tests and even clean up afterwards.

wdyt?

thank you. I agree. that is a clean approach.

evankanderson · 2024-10-08T13:21:17Z

internal/util/helpers_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			err := os.Setenv("XDG_CONFIG_HOME", tt.envVar)


Doing this in t.Parallel may cause interference with other tests.

What I've done in the past is to put some explicit locks on these tests; without that, these tests will tend to be flaky when running on multiple cores at the same time.

Additionally, you'll want to reset the value of XDG_CONFIG_HOME after this test. You can use t.Setenv() to do this.

evankanderson · 2024-10-08T13:27:00Z

internal/util/helpers_test.go

+	xdgConfigHome := os.Getenv("XDG_CONFIG_HOME")
+	if xdgConfigHome == "" {
+		homeDir, err := os.UserHomeDir()
+		if err != nil {
+			t.Fatalf("error getting home directory: %v", err)
+		}
+		xdgConfigHome = filepath.Join(homeDir, ".config")
+	}
+
+	filePath := filepath.Join(xdgConfigHome, "minder", "credentials.json")


This environment-related stuff is going to conflict with TestGetGrpcConnection; it will also end up touching the user's actual home directory.

What I would do:

Use t.TempDir() to create a temp directory.

Set XDG_CONFIG_HOME to point at the directory from (1).

Thank you. it is good to learn about the impact to other tests.

evankanderson · 2024-10-08T13:27:51Z

internal/util/helpers_test.go

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			err := os.Setenv(MinderAuthTokenEnvVar, tt.envToken)


Again, this use of env vars is likely to cause trouble. At a minimum, you should restore the previous value of the environment variable at the end of the t.Run() function (e.g. with defer)

evankanderson · 2024-10-08T13:32:16Z

internal/util/helpers_test.go

+			parsedURL, _ := url.Parse(server.URL)
+			tt.issuerUrl = parsedURL.String()
+
+			result, err := RefreshCredentials(tt.refreshToken, tt.issuerUrl, tt.clientId)


Since this function is not side-effect-free, you may need to again set XDG_CONFIG_HOME to point to a different location.

evankanderson · 2024-10-08T13:32:45Z

internal/util/helpers_test.go

+			originalEnv := os.Getenv("XDG_CONFIG_HOME")
+			err = os.Setenv("XDG_CONFIG_HOME", tempDir)


t.Setenv() will do this for you.

evankanderson · 2024-10-08T14:04:11Z

internal/util/helpers_test.go

+
+			// Create a temporary file for testing
+			if tc.filePath == "testfile.txt" {
+				err := os.WriteFile(tc.filePath, []byte(tc.expectedDesc), 0600)


We should do this in t.TestDir()

Thank you. I assume, you were referring to t.TempDir()

Yes, thanks!

evankanderson · 2024-10-23T13:06:47Z

@gajananan

It looks like this is close to the finish line -- any chance you can incorporate the feedback (and fix conflicts) in the next week?

evankanderson · 2024-11-06T14:06:47Z

Hi @gajananan,

We'd love to get this merged! If you don't have time to work on this, we'll probably have someone over here at Stacklok apply the requested changes and merge it.

evankanderson · 2024-11-19T14:06:41Z

Hey @gajananan -- we'd love to get this PR in. I ran into one of your co-workers at KubeCon, and they said that your work schedule might be preventing you from updating. A quick note of "I can get to this within a week" or "please carry this forward, I'm busy now" would be handy.

Thanks again for your work so far!

github-actions · 2024-12-20T02:33:04Z

This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days.

Add additional test coverage for helper functions in Util

1395938

Currently helper functions in Util lacks test coverage This commit adds test coverage helper functions in Util Signed-off-by: Kugamoorthy Gajananan <[email protected]>

gajananan added 5 commits October 6, 2024 11:23

Add additional test coverage for helper functions in Util

8b57118

Currently helper functions in Util lacks test coverage This commit fixes the lint errors and handles errors. Signed-off-by: Kugamoorthy Gajananan <[email protected]>

Add additional test coverage for helper functions in Util

d49f7b7

Currently helper functions in Util lacks test coverage. This commit - fixes the lint errors and handles errors. - Adds test coverage for LoadCredentials function Signed-off-by: Kugamoorthy Gajananan <[email protected]>

Add additional test coverage for helper functions in Util

00dd4ba

Currently helper functions in Util lacks test coverage This commit fixes the lint errors after fixing lint version. Signed-off-by: Kugamoorthy Gajananan <[email protected]>

Add additional test coverage for helper functions in Util

c671a66

Currently helper functions in Util lacks test coverage This commit adds test coverage for RevokeToken function. Signed-off-by: Kugamoorthy Gajananan <[email protected]>

gajananan marked this pull request as ready for review October 8, 2024 12:15

JAORMX reviewed Oct 8, 2024

View reviewed changes

evankanderson reviewed Oct 8, 2024

View reviewed changes

github-actions bot added the Stale label Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional test coverage for helper functions in Util #4651

Add additional test coverage for helper functions in Util #4651

gajananan commented Oct 5, 2024 •

edited

Loading

coveralls commented Oct 5, 2024 •

edited

Loading

JAORMX Oct 8, 2024

gajananan Oct 8, 2024

evankanderson Oct 8, 2024

evankanderson Oct 8, 2024

evankanderson Oct 8, 2024

gajananan Oct 9, 2024

evankanderson Oct 8, 2024

evankanderson Oct 8, 2024

evankanderson Oct 8, 2024

evankanderson Oct 8, 2024

gajananan Oct 8, 2024

evankanderson Oct 8, 2024

evankanderson commented Oct 23, 2024

evankanderson commented Nov 6, 2024

evankanderson commented Nov 19, 2024

github-actions bot commented Dec 20, 2024

		originalEnv := os.Getenv("XDG_CONFIG_HOME")
		err = os.Setenv("XDG_CONFIG_HOME", tempDir)

Add additional test coverage for helper functions in Util #4651

Are you sure you want to change the base?

Add additional test coverage for helper functions in Util #4651

Conversation

gajananan commented Oct 5, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

coveralls commented Oct 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson commented Oct 23, 2024

evankanderson commented Nov 6, 2024

evankanderson commented Nov 19, 2024

github-actions bot commented Dec 20, 2024

gajananan commented Oct 5, 2024 •

edited

Loading

coveralls commented Oct 5, 2024 •

edited

Loading