Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dont load users zshrc file during test runs #82

Closed
MichaelAquilina opened this issue Aug 13, 2017 · 10 comments
Closed

Dont load users zshrc file during test runs #82

MichaelAquilina opened this issue Aug 13, 2017 · 10 comments
Labels

Comments

@MichaelAquilina
Copy link
Contributor

MichaelAquilina commented Aug 13, 2017

Tests should be independent from what's contained within the users zsh configuration as it can interfere with the test process. In short, tests should not load the user's zshrc profile when running tests.

(PS: Only just realised I had an invitation pending for me to join this project, sorry about that!)

@molovo
Copy link
Member

molovo commented Aug 14, 2017

@MichaelAquilina I'll have to double check later, but I don't think we are loading .zshrc. Since ZUnit is an executable and ZSH doesn't source .zshrc for those it would have to be done explicitly in ZUnit's code which I'm pretty sure it's not doing. It will source .zshenv, which is probably useful since it's commonly used to populate $path and $fpath which will be needed to access external commands the app being tested depends on. If we decided to bypass loading all environment files, the only way I know to do that would be to replace the #!/usr/bin/env zsh shebang with #!/usr/bin/zsh -f, but that introduces further issues if ZSH is not at that exact path. I'm pretty sure #!/usr/bin/env zsh -f doesn't work since the -f option is passed to env and not zsh. Do you have any other ideas as to how we might do it?

@MichaelAquilina
Copy link
Contributor Author

Hmmm maybe I'll doubel check that I actually experienced this. If I manage to find a reproducible scenario I'll post it here :)

@MichaelAquilina
Copy link
Contributor Author

So the way I'm testing this is with the following minimal test case:

#!/usr/bin/env zunit

@test 'test something' {
    run env
}

I then run zunit --verbose test_something.zunit to check the output of env. There are variables set in the output that are definitely from my zshrc (plugins, custom variables etc)

Is this expected?

@MichaelAquilina
Copy link
Contributor Author

This patch also shows the same issue (run with --verbose)

diff --git a/src/commands/run.zsh b/src/commands/run.zsh
index f2f8eac..4a6ebc8 100644
--- a/src/commands/run.zsh
+++ b/src/commands/run.zsh
@@ -100,6 +100,7 @@ function _zunit_execute_test() {
 
       # The test body is printed here, so when we eval the wrapper
       # function it will be read as part of the body of this function
+      env
       ${body}
 
       # If a teardown function is defined, run it now

@molovo
Copy link
Member

molovo commented Aug 14, 2017

Variables yes, since they're inherited by child processes. Aliases and functions defined in .zshrc shouldn't be defined in your tests though as the file isn't sourced. There's no way of removing those environment variables without manually calling unset - even running ZSH with the -f flag will retain variables set in the parent process, that's how they're designed to work. The only way to run ZUnit without environment variables is to launch zsh -f as a brand new process by changing the default shell command used when terminal launches, not from another shell session, and then run ZUnit. Even if you were to launch zsh -f from bash you'd inherit any variables set in the bash environment. Of course there might be another flag to prevent that behaviour that I'm not aware of - I'll have a trawl through the man pages

@MichaelAquilina
Copy link
Contributor Author

I wasnt aware of that! I found that you can run env -i command to run command without any previously set environment variables. Do you think that would be helpful or do you think the current way it works is intended behaviour?

@molovo
Copy link
Member

molovo commented Aug 14, 2017

If we can get it working it would be a great feature, but I wouldn't want to make it the default, as it would break expected POSIX behaviour. It might work to add a --no-env flag, which launches the test processes behind env -i within ZUnit.

That could be difficult though - since ZUnit declares it's own vars and functions internally which are then cascaded down to the test level (including the test functions themselves), we'd lose those as well, and I don't know at what level we'd be able to do that without breaking things.

Perhaps we could listen for the flag, then if it's set, internally we end processing, and relaunch the entire process using something like $(env -i $0 "$@"). Then all the external variables would be stripped, and internal variables would be redeclared as the process begins at the top of the tree.

@MichaelAquilina
Copy link
Contributor Author

You are right, thinking about it, many things would break with the environment unset 🤔

@molovo
Copy link
Member

molovo commented Aug 14, 2017

Yeah, having thought about it some more, we'd lose $PATH, $ZDOTDIR and $ZSH_VERSION, which would completely break things for almost every ZSH script out there. If there's a particular variable that's causing you issues, I'd recommend creating a bootstrap script which unsets any variables which affect your script specifically, and then you've got a blank slate, but i think anything more brutal than that will do more harm than good.

@MichaelAquilina
Copy link
Contributor Author

Yeah that all makes sense to me. Thanks for the help and feedback @molovo! I think its safe to mark this issue as closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants