zunit-zsh / zunit

A powerful testing framework for ZSH projects
https://zunit.xyz
MIT License
208 stars 23 forks source link

Dont load users zshrc file during test runs #82

Closed MichaelAquilina closed 7 years ago

MichaelAquilina commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

molovo commented 7 years ago

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 commented 7 years ago

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