Update shell rcfiles to indicate the user is already in an activated state. #2710

mitchell-as · 2023-08-16T19:02:03Z

DX-2045 As a user I can expect to always see a clear indication that I am in a virtual environment

The PS1 prompt is only changed for the immediate subshell. Nested subshells do not inherit it, so it was not clear the user is still in an activated state.

cmd.exe does not need this because its PS1 equivalent is unmodified when opening another shell within a shell. I've also added a comment to the JIRA ticket explaining why I don't want to satisfy the Windows AC.

…state. The PS1 prompt is only changed for the immediate subshell. Nested subshells do not inherit it, so it was not clear the user is still in an activated state.

internal/assets/contents/shells/fishrc_append.fish

internal/subshell/sscommon/rcfile_test.go

test/integration/shell_int_test.go

Naatan

See comment replies

This allows for testing bare 'bash' and 'zsh' commands. Also revert fish to zsh test change.

It's not clear which shell CI will pick up by default on macOS (sometimes bash, sometimes zsh), so touch to make sure.

Naatan · 2023-08-21T17:05:28Z

test/integration/shell_int_test.go

@@ -288,13 +291,19 @@ func (suite *ShellIntegrationTestSuite) TestNestedShellNotification() {

 	cfg, err := config.New()
 	suite.Require().NoError(err)
+	if runtime.GOOS == "darwin" && condition.OnCI() {
+		cfg.Set(subshell.ConfigKeyShell, "zsh") // GitHub actions runs on bash, so override with zsh


I'd rather we use the SHELL=zsh environment variable, as it's more future-proof.

We actually cannot do this because os.Setenv("SHELL") will affect parallel tests. I reverted a previous change.

Naatan · 2023-08-21T17:06:48Z

test/integration/shell_int_test.go

+	os.Setenv(constants.HomeEnvVarName, ts.Dirs.HomeDir)
+	defer func() { os.Unsetenv(constants.HomeEnvVarName) }()


This is not safe for other tests running in parallel. You can set it with AppendEnv though.

I've tried to minimize time spent in a modified env state. It's needed for user.HomeDir() both in this test and spawned processes.

Ok, please find an alternative to using ss.RcFile() that doesn't require os.Setenv(). This is just a test so it's ok if we're hard coding some stuff here so long as it's not prone to breaking all the time.

As for the process-level; AppendEnv() should be able to handle that.

Minimizing the window doesn't change that it can impact other tests in ways that will be really hard to identify after the fact.

I've hardcoded creating and populating shellrc files like the installer would.

Naatan · 2023-08-21T17:08:06Z

test/integration/shell_int_test.go

 	ss := subshell.New(cfg)
 	err = subshell.ConfigureAvailableShells(ss, cfg, nil, sscommon.InstallID, true) // mimic installer
 	suite.Require().NoError(err)


Why is this necessary for these tests?

This is needed to put the .zshrc and .bashrc script in the test userhome directory. If I don't have this, the test will fail with ".zshrc not found".

Can we instead just "touch" those files? This feels like we're throwing too much at it. Especially in tests the variables should be as minimal as possible.

I've hardcoded creating and populating shellrc files like the installer would.

Using defer may interfere with parallel tests.

Naatan

See comment responses.

…sage.

…rror message." This reverts commit 1cf9727.

Naatan · 2023-08-22T17:08:32Z

test/integration/shell_int_test.go

 		ss = subshell.New(cfg)
 	} else {
-		os.Setenv("SHELL", "zsh") // GitHub actions runs on bash, so override with zsh
+		cfg.Set(subshell.ConfigKeyShell, "zsh")


Please get rid of the subshell package in these tests. We can just make assumptions based on OS. eg. Mac = zsh, Linux = bash, Windows = cmd.

The subshell package adds too much logic to these tests imo. I'd rather keep this dead simple and hardcoded so the tests leave little to the imagination and we don't accidentally break them without realizing it when eg. updating the subshell package.

Forgive me. I was under the impression "integration tests" were meant to test as much of the existing system in-place as possible, so breaking changes introduced in the subshell packages might be caught here.

Our integration tests are meant to test the built state tool. They are not meant to test the code directly, that's what unit tests are for.

Testing it like this itself can also cause issues where the test passes when in fact it shouldn't, because we are using the same code to assert as we are testing, this code would have the same bugs.

Naatan · 2023-08-22T17:11:57Z

test/integration/shell_int_test.go

 	suite.Require().Equal(filepath.Dir(rcFile), ts.Dirs.HomeDir, "rc file not in test suite homedir")
-	suite.Require().FileExists(rcFile)
+	fileutils.TouchFileUnlessExists(rcFile)
+	sscommon.WriteRcFile(rcFileAppend, rcFile, sscommon.InstallID, nil)


I don't follow why we need this? The TouchFile above seems to already be addressing the needs of this test. And without passing any env information this wouldn't actually be writing any templated data. The InstallID is also irrelevant because you're using the append template, which doesn't use this.

Pretty sure you can drop this and your test would still run as expected.

If I killed line 320 as you appear to suggest, the test would fail because the shell rc file would not contain the templated if statement that checks and prints if you are in a subshell: https://github.com/ActiveState/cli/pull/2710/files#diff-7d9693c16c57a7c68d369e96c2969510a7193762c48b65c36e49b9d789669e2b.

Ahhh that's the part I was missing. I was looking at my local zshrc_append file and wondering what you were looking to set up -.-

mitchell-as · 2023-08-22T19:41:35Z

I expect more churn.

Naatan

Sorry; I was not fully up to speed on what the test was trying to do. I now understand you need to set up the bashrc file with the new modifications you have introduced in this PR. This would normally happen as part of the installation, which it makes sense you want to avoid as part of this test.

Now that I understand this; it does seem like calling ConfigureAvailableShells() is the most appropriate solution, as that matches what the installer does. I apologize as this was how you had done it at first; I sent us on a wild goose-chase and I'll own that.

I think long term we should actually do an "install" for a test like this, but at the moment all our install/update testing is already so fragile that it doesn't make sense to add to that problem.

mitchell-as · 2023-08-23T14:43:18Z

I've filed DX-2153 to do what you suggest. I've already spent too much time on this ticket. As outlined in that ticket I filed, reverting back to subshell.ConfigureAvailableShells() requires overrides to the home directory to install shell rc files to, and the overrides are currently at an environment variable level. Changing this may affect other integration tests running in parallel as you pointed out previously.

I am open to tweaking what I currently have in this PR, but I don't want to revert too much because I feel it's going to waste more of our time.

Naatan

Let's go with these changes. I forgot about the Setenv call; that is definitely something worth avoiding. Reading through the code it doesn't look too complicated to make subshell take a "home dir" argument in its constructor or something along those lines, but that is going to lead to refactorings which does feel like a bit of a scope creep. The story you filed sufficiently addresses the concern already, let's just address it there.

Thanks for bearing with me.

This reverts commit 796ddea, reversing changes made to 59bbb9c.

mitchell-as added the Test: shell label Aug 16, 2023

mitchell-as force-pushed the mitchell/dx-2045 branch 3 times, most recently from cabcef3 to dff11c9 Compare August 16, 2023 20:37

Update shell rcfiles to indicate the user is already in an activated …

3cfa3a5

…state. The PS1 prompt is only changed for the immediate subshell. Nested subshells do not inherit it, so it was not clear the user is still in an activated state.

mitchell-as force-pushed the mitchell/dx-2045 branch from dff11c9 to 3cfa3a5 Compare August 16, 2023 20:53

mitchell-as requested a review from Naatan August 16, 2023 21:24

mitchell-as marked this pull request as ready for review August 16, 2023 21:24

Naatan requested changes Aug 17, 2023

View reviewed changes

internal/assets/contents/shells/fishrc_append.fish Outdated Show resolved Hide resolved

internal/subshell/sscommon/rcfile_test.go Outdated Show resolved Hide resolved

test/integration/shell_int_test.go Outdated Show resolved Hide resolved

Improve readability of fish shell conditional.

39efdf8

mitchell-as requested a review from Naatan August 17, 2023 21:17

Naatan requested changes Aug 17, 2023

View reviewed changes

mitchell-as force-pushed the mitchell/dx-2045 branch 2 times, most recently from 2f61415 to 1cbc286 Compare August 18, 2023 15:26

Set HOME env var for nested shell notification tests.

b065046

This allows for testing bare 'bash' and 'zsh' commands. Also revert fish to zsh test change.

mitchell-as force-pushed the mitchell/dx-2045 branch 2 times, most recently from a1cd38c to 43f9213 Compare August 18, 2023 16:44

Use zsh instead of bash in macOS integration tests.

71ad314

mitchell-as force-pushed the mitchell/dx-2045 branch from 43f9213 to 71ad314 Compare August 18, 2023 17:48

Do not assume default shellrc file exists.

10f924c

It's not clear which shell CI will pick up by default on macOS (sometimes bash, sometimes zsh), so touch to make sure.

mitchell-as requested a review from Naatan August 18, 2023 19:29

Naatan requested changes Aug 21, 2023

View reviewed changes

mitchell-as added 2 commits August 21, 2023 13:29

Prefer overriding SHELL env var to set shell instead of changing config.

9186bc2

Minimize time spent with modified process env.

1a673fd

Using defer may interfere with parallel tests.

mitchell-as requested a review from Naatan August 21, 2023 19:31

Naatan requested changes Aug 21, 2023

View reviewed changes

mitchell-as added 2 commits August 21, 2023 15:53

Hardcode shell-specific files in integration test based on PR feedback.

4966d17

Revert changing SHELL in current process to avoid parallel test issues.

0d5e963

mitchell-as requested a review from Naatan August 21, 2023 20:13

Use correct project and directory in "already active shell" error mes…

1cf9727

…sage.

Revert "Use correct project and directory in "already active shell" e…

ca096f0

…rror message." This reverts commit 1cf9727.

Naatan requested changes Aug 22, 2023

View reviewed changes

More hardcoded shell stuff.

55f933e

mitchell-as force-pushed the mitchell/dx-2045 branch from 36a8781 to 55f933e Compare August 22, 2023 19:25

mitchell-as requested a review from Naatan August 22, 2023 19:41

Naatan requested changes Aug 22, 2023

View reviewed changes

mitchell-as requested a review from Naatan August 23, 2023 14:48

Naatan approved these changes Aug 23, 2023

View reviewed changes

mitchell-as merged commit 796ddea into version/0-41-0-RC1 Aug 23, 2023

mitchell-as deleted the mitchell/dx-2045 branch August 23, 2023 16:17

as-builds added a commit that referenced this pull request Aug 23, 2023

Merge branch version/0-41-0-RC1 to adopt changes from PR #2710

c69815a

mitchell-as added a commit that referenced this pull request Oct 3, 2023

Revert "Merge pull request #2710 from ActiveState/mitchell/dx-2045"

3ef3ae4

This reverts commit 796ddea, reversing changes made to 59bbb9c.

mitchell-as mentioned this pull request Oct 3, 2023

Revert: Update shell rcfiles to indicate the user is already in an activated state (v41). #2787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update shell rcfiles to indicate the user is already in an activated state. #2710

Update shell rcfiles to indicate the user is already in an activated state. #2710

mitchell-as commented Aug 16, 2023 •

edited by github-actions bot

Loading

Naatan left a comment

Naatan Aug 21, 2023

mitchell-as Aug 21, 2023 •

edited

Loading

Naatan Aug 21, 2023

mitchell-as Aug 21, 2023 •

edited

Loading

Naatan Aug 21, 2023

mitchell-as Aug 21, 2023

Naatan Aug 21, 2023

mitchell-as Aug 21, 2023

Naatan Aug 21, 2023

mitchell-as Aug 21, 2023

Naatan left a comment

Naatan Aug 22, 2023

mitchell-as Aug 22, 2023

Naatan Aug 22, 2023

Naatan Aug 22, 2023

mitchell-as Aug 22, 2023

Naatan Aug 22, 2023

mitchell-as commented Aug 22, 2023

Naatan left a comment

mitchell-as commented Aug 23, 2023

Naatan left a comment

		os.Setenv(constants.HomeEnvVarName, ts.Dirs.HomeDir)
		defer func() { os.Unsetenv(constants.HomeEnvVarName) }()

Update shell rcfiles to indicate the user is already in an activated state. #2710

Update shell rcfiles to indicate the user is already in an activated state. #2710

Conversation

mitchell-as commented Aug 16, 2023 • edited by github-actions bot Loading

Naatan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as Aug 21, 2023 • 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

Naatan left a 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

mitchell-as commented Aug 22, 2023

Naatan left a comment

Choose a reason for hiding this comment

mitchell-as commented Aug 23, 2023

Naatan left a comment

Choose a reason for hiding this comment

mitchell-as commented Aug 16, 2023 •

edited by github-actions bot

Loading

mitchell-as Aug 21, 2023 •

edited

Loading

mitchell-as Aug 21, 2023 •

edited

Loading