Add IssueFileCommand & update Set Env, Add Path to use latest standard #11

ashutoshgngwr · 2020-10-15T14:05:09Z

All the necessary details for the change is well documented in #9 but the overall gist of it can be found in this document alone.

fixes sethvargo#9

ashutoshgngwr · 2020-10-15T14:10:09Z

Weird! Tests are passing on my machine, failing in CI.

Also works in a clean Docker container.

Sorry my bad, these tests rely on the GITHUB_ENV and GITHUB_PATH variables which is being provided by the CI environment. Let me fix it.

sethvargo · 2020-10-15T14:44:06Z

actions.go

+	if err != nil {
+		return fmt.Errorf(errFileCmdFmt, err)
+	}
+


Need to defer closing the file. Also need to sync and flush with error handling. Alternatively, use ioutil.WriteFile

Oh missed it! I think ioutil.WriteFile sounds cleaner. I'll fix this!

actions.go

sethvargo · 2020-10-15T14:44:52Z

actions.go

@@ -208,7 +259,8 @@ func (c *Action) Warningf(msg string, args ...interface{}) {
 }

 // WithFieldsSlice includes the provided fields in log output. "f" must be a
-// slice of k=v pairs. The given slice will be sorted.
+// slice of k=v pairs. The given slice will be sorted. It panics if any of the


sethvargo · 2020-10-15T14:46:18Z

actions_test.go

@@ -73,13 +116,51 @@ func TestAction_RemoveMatcher(t *testing.T) {
 func TestAction_AddPath(t *testing.T) {
 	t.Parallel()

+	const envGitHubPath = "GITHUB_PATH"


It's generally a smell to set/unset envvars in tests, as it causes races when run in parallel. Can you tell me more about what this is attempting to do? I might be able to suggest an alternative approach.

So essentially with new file commands, GitHub actions populate the environment with GITHUB_{CMD} variables, e.g. GITHUB_PATH with a file path where command data needs to be written. This is basically to test our code's behaviour end-to-end. We pass the same environment variable with a file path to it, that we expected from the GitHub runner. I originally didn't unset the env variables so when the tests ran in GitHub action environment, they failed because first part - of the tests - tests the fallback commands in absence of the variable. The issue obviously was that these variables were being populated by the runner.

Could we just set GITHUB_PATH="" inside the action.yml instead?

I think we can. Let me try!

@sethvargo We're still stuck with 1 Setenv call per test for three test cases. The reason being that IssueFileCommand expects the file path to be passed as an environment variable, and AddPath and SetEnv are both dependent on it. Although resetting required env vars in workflow config does remove the reset/restore calls in tests, they still need to set for completing the tests.

Could you go through AddPath test again?

go-githubactions/actions_test.go

Lines 116 to 164 in a03a582

func TestAction_AddPath(t *testing.T) {

t.Parallel()

const envGitHubPath = "GITHUB_PATH"

defer os.Setenv(envGitHubPath, os.Getenv(envGitHubPath))

os.Unsetenv(envGitHubPath)

// expect a regular command to be issued when env file is not set.

var b bytes.Buffer

a := NewWithWriter(&b)

a.AddPath("/custom/bin")

if got, want := b.String(), "::add-path::/custom/bin\n"; got != want {

t.Errorf("expected %q to be %q", got, want)

}

// expect a file command to be issued when env file is set.

file, err := ioutil.TempFile(".", ".add_path_test_")

if err != nil {

t.Fatalf("unable to create a temp env file: %s", err)

}

defer os.Remove(file.Name())

if err = os.Setenv(envGitHubPath, file.Name()); err != nil {

t.Fatalf("unable to set %q env var: %s", envGitHubPath, err)

}

b.Reset()

a.AddPath("/custom/bin")

if got, want := b.String(), ""; got != want {

t.Errorf("expected %q to be %q", got, want)

}

// expect an empty stdout buffer

if got, want := b.String(), ""; got != want {

t.Errorf("expected %q to be %q", got, want)

}

// expect the message to be written to the file.

data, err := ioutil.ReadAll(file)

if err != nil {

t.Errorf("unable to read temp env file: %s", err)

}

if got, want := string(data), "/custom/bin\n"; got != want {

t.Errorf("expected %q to be %q", got, want)

}

}

In words, it does the following:

Unset GITHUB_PATH (provided by the GitHub runner)

Call AddPath() since GITHUB_PATH is unset, the test expects the fallback command to execute

Create a temp file

Set GITHUB_PATH to the temp file path

Call AddPath() again and expect the file command to execute

Verify if command data was written to the temp file

File cleanup and env restoration

SetEnv test also follows the same pattern. Now even if we eliminate the Unset call, we still have to populate GITHUB_PATH with the temp file path.

For testing, it's usually common to add another function like IssueFileCommandLookup that has the same API but accepts an interface that responds to what you want (Getenv in this case). That way, for tests, you can create a struct that implements that function on a map, and then IssueFileCommand becomes:

IssueFileCommandLookup(cmd, os.Getenv)

Does that make sense?

I thought of that at some point. Lacked the confidence to carry it out in effect. lol

ashutoshgngwr · 2020-10-15T18:48:55Z

@sethvargo On an unrelated note, for both Add-Path and Set-Env commands, the TypeScript toolkit ensures that the effects are visible to the current process immediately after execution. Should I go ahead and add it here as well?

https://github.com/actions/toolkit/blob/af821474235d3c5e1f49cee7c6cf636abb0874c4/packages/core/src/core.ts#L74
https://github.com/actions/toolkit/blob/af821474235d3c5e1f49cee7c6cf636abb0874c4/packages/core/src/core.ts#L43

ashutoshgngwr · 2020-10-20T06:24:34Z

@sethvargo ping

sethvargo · 2020-10-20T16:52:09Z

Ugh sorry - I forgot to click "Submit Review" 😦

ashutoshgngwr · 2020-10-20T18:32:52Z

actions.go

+	return c.issueFileCommand(cmd, os.Getenv)
+}
+
+func (c *Action) issueFileCommand(cmd *Command, f getenvFunc) error {


I have created unexported functions in-place of suggested IssueFileCommandLookup. IMO, exporting these functions doesn't create any value. I've also found this pattern to be a common occurrence in Kubernetes code-base. Please let me know if you think that these should be exported functions instead.

Unexported is fine. Less to worry about maintaining/breaking in the future 😄

sethvargo · 2020-10-20T19:52:42Z

actions.go

+	return c.issueFileCommand(cmd, os.Getenv)
+}
+
+func (c *Action) issueFileCommand(cmd *Command, f getenvFunc) error {


Unexported is fine. Less to worry about maintaining/breaking in the future 😄

sethvargo · 2020-10-20T19:53:46Z

actions_test.go

+// newFakeGetenvFunc returns a new getenvFunc that is expected to be called with
+// the provided key. It returns the provided value if the call matches the
+// provided key. It reports an error on test t otherwise.
+func newFakeGetenvFunc(t *testing.T, wantKey, v string) getenvFunc {


Nit: you generally want to accept testing.TB instead of *testing.T so that the interface works with both benchmarks and tests.

github-actions · 2020-11-04T00:43:24Z

This pull request has been automatically locked since there has not
been any recent activity after it was closed. Please open a new
issue for related bugs.

ashutoshgngwr added 2 commits October 15, 2020 19:34

Add IssueFileCommand & update SetEnv, AddPath to latest standard

5d0a808

fixes sethvargo#9

Add docs for the panic in Action.WithFieldsSlice

549b267

ashutoshgngwr added 2 commits October 15, 2020 19:50

Fix incorrect error formatting in Action tests

b80714f

Fix breaking tests in actions_test

a03a582

sethvargo requested changes Oct 15, 2020

View reviewed changes

Use ioutil.WriteFile in Action.IssueFileCommand

b9df1b4

Add docs explaining the usage of deprecated commands as fallbacks

f31f8e5

Abstract os.Getenv to avoid fiddling with actual env during tests

bddfa21

ashutoshgngwr commented Oct 20, 2020

View reviewed changes

ashutoshgngwr requested a review from sethvargo October 20, 2020 18:33

sethvargo approved these changes Oct 20, 2020

View reviewed changes

sethvargo merged commit e057b58 into sethvargo:main Oct 20, 2020

github-actions bot locked as resolved and limited conversation to collaborators Nov 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IssueFileCommand & update Set Env, Add Path to use latest standard #11

Add IssueFileCommand & update Set Env, Add Path to use latest standard #11

ashutoshgngwr commented Oct 15, 2020

ashutoshgngwr commented Oct 15, 2020 •

edited

sethvargo Oct 15, 2020

ashutoshgngwr Oct 15, 2020

sethvargo Oct 15, 2020

sethvargo Oct 15, 2020

ashutoshgngwr Oct 15, 2020

sethvargo Oct 15, 2020

ashutoshgngwr Oct 15, 2020

ashutoshgngwr Oct 15, 2020

sethvargo Oct 15, 2020

ashutoshgngwr Oct 20, 2020

ashutoshgngwr commented Oct 15, 2020

ashutoshgngwr commented Oct 20, 2020

sethvargo commented Oct 20, 2020

ashutoshgngwr Oct 20, 2020

sethvargo Oct 20, 2020

sethvargo Oct 20, 2020

sethvargo Oct 20, 2020

github-actions bot commented Nov 4, 2020

	func TestAction_AddPath(t *testing.T) {
	t.Parallel()

	const envGitHubPath = "GITHUB_PATH"
	defer os.Setenv(envGitHubPath, os.Getenv(envGitHubPath))
	os.Unsetenv(envGitHubPath)

	// expect a regular command to be issued when env file is not set.
	var b bytes.Buffer
	a := NewWithWriter(&b)

	a.AddPath("/custom/bin")
	if got, want := b.String(), "::add-path::/custom/bin\n"; got != want {
	t.Errorf("expected %q to be %q", got, want)
	}

	// expect a file command to be issued when env file is set.
	file, err := ioutil.TempFile(".", ".add_path_test_")
	if err != nil {
	t.Fatalf("unable to create a temp env file: %s", err)
	}

	defer os.Remove(file.Name())
	if err = os.Setenv(envGitHubPath, file.Name()); err != nil {
	t.Fatalf("unable to set %q env var: %s", envGitHubPath, err)
	}

	b.Reset()
	a.AddPath("/custom/bin")

	if got, want := b.String(), ""; got != want {
	t.Errorf("expected %q to be %q", got, want)
	}

	// expect an empty stdout buffer
	if got, want := b.String(), ""; got != want {
	t.Errorf("expected %q to be %q", got, want)
	}

	// expect the message to be written to the file.
	data, err := ioutil.ReadAll(file)
	if err != nil {
	t.Errorf("unable to read temp env file: %s", err)
	}

	if got, want := string(data), "/custom/bin\n"; got != want {
	t.Errorf("expected %q to be %q", got, want)
	}
	}

Add IssueFileCommand & update Set Env, Add Path to use latest standard #11

Add IssueFileCommand & update Set Env, Add Path to use latest standard #11

Conversation

ashutoshgngwr commented Oct 15, 2020

ashutoshgngwr commented Oct 15, 2020 • edited

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

ashutoshgngwr commented Oct 15, 2020

ashutoshgngwr commented Oct 20, 2020

sethvargo commented Oct 20, 2020

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

github-actions bot commented Nov 4, 2020

ashutoshgngwr commented Oct 15, 2020 •

edited