unset BSD_ECHO for Zsh features

[e2]

Summary

Make Zsh shell scripts work regardless of local user Zsh configuration.

Details

Some scenarios depend on echo allowing escape sequences.

From the Zsh manual:

BSD_ECHO
Make the echo builtin compatible with the BSD echo(1) command. This disables backslashed escape sequences in echo strings unless the `-e' option is specified.

Motivation and Context

Having setopt BSD_ECHO in Zsh config files (like I did) causes shell-related scenarios to fail.
Making sure this option is unset allows tests to work locally, even if this is set.

How Has This Been Tested?

Local tests now work. Doesn't affect anything other than those few previously-failing cucumber scenarios.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code follows the code style of this project.

[ghost]

Can you try to do this instead? I don't want to add things to tests which are not relevant to them.

Background:
# [...]
 Given I set the environment variable "ZDOTDIR" to "/tmp/"

Scenario: ...

See http://linuxcommand.org/man_pages/zshall1.html for mor information about ZDOTDIR.

       ZDOTDIR
              The  directory  to search for shell startup files (.zshrc, etc),

[e2]

I don't want to add things to tests which are not relevant to them.

If we take that one step further:

1. I have no idea what those tests are really testing - it looks like just absolute path vs relative path to binary
2. I don't see a point in even using zsh or javac or python or even bash at all. Or it would make sense to reset things for all of them.
3. I don't see a point in testing the echo \c things. It looks like Aruba is testing Zsh.

Those scenarios are to test Aruba::ScriptFile.new, so what really should be tested is:

1. Whether shebang in script is honored (you don't need zsh, javac, etc. for this)
2. Whether arguments are properly passed to the script (you don't need external utilities for this)
3. Whether the interpreter uses an absolute path or not

[e2]

In short, I'd accept this PR for now, because tests are broken and this fixes it.

Spending time to switch to paths doesn't make sense, because it's just using a different system resource - /tmp dir.
(It doesn't change the fact that zsh is necessary - while it shouldn't be).

The "best solution" requires rewriting those whole Cucumber scenarios. I don't really have time for that. (And neither did I write those features in the first place).

So I shouldn't be "punished" with reworking those scenarios - just to stop the tests from failing on my setup (a bigger priority than "combing" a few scenarios that need to be rewritten shortly anyway - to drop dependency on those external binaries, and, to reduce the Docker container size/time).

There's no need for contributors of Aruba to install Zsh just to get 1-2 scenarios working. So there's no need to "fix a zsh setup" if Zsh will be removed soon anyway. (It's just a waste of precious time).

[ghost]

@e2 Not meant to "punish" you!

We decided to use cucumber/aruba-features for our documentation as well! That's the reason why those features exist. They demonstrate that one can generate whatever script language one prefers. From my point of view that's kind of living documentation. Personally I like writing documentation (sometimes), but then I've got the problem that it's outdated.

Choosing cucumber/aruba for this job shows the "maintainer/developer" which documentation needs to be fixed since this is kind of contract between the project and it's users: We promise our users, that the product behaves like it was documented.

I like the way of living documentation and for now it's what have been agreed upon internally. I'm open to suggestion how we can improve that. I also asked cucumber-ruby-maintainers for adivce - cucumber/cucumber-ruby#964. Maybe we "flag" those features and run them only on CI.

[e2]

The root problem here is: using binaries from outside of the project.

This is going to come and bite again - once someone starts to dive into Windows problems, where almost every binary will either not be present, or will behave differently. Using Travis for debugging Windows problems isn't very exciting.

The best approach would be to have "shims", and substitute paths (which is actually ALREADY being done by Aruba). E.g. if Aruba calls /usr/bin/zsh, it should actually run aruba/tmp/cli-app/usr/bin/zsh - and that "zsh" could be a simple script that provide the value needed.

The problem with the current approach is e.g: that you can have path-handling logic FAILING in Aruba, and you won't know.

E.g. what if a user has zsh in ~/bin, /usr/local/bin and /usr/bin/ and the WRONG one is called (according to expectations). Currently, if that's the case, the output may be the same, either way. (So you may not notice if there's an error).

But, if you have aruba/tmp/cli-app/usr/local/bin/zsh outputting an error when it's called as /usr/bin/zsh, you can catch that error.

It all depends on what is being tested. It's definitely not "compatibility with popular scripting languages". Because Aruba actually doesn't handle that. (Zsh - case in point).

So if Aruba needs custom setup and custom expectations in JUST the tests for these tools ... replacing those binaries with "stubs" is the way to go.

Especially if the value is "documentation". We're not testing "output handling" and we're not testing "if Zsh works on this system".

The problem with such "shims" is: they're ugly. But, they do make the test more robust and offer a lot more options and control when it comes to testing.

So I'm not sure it's worth working on them if the PRs will be rejected solely because "they're ugly".

And compared to how "ugly" they are and how "problematic" dealing with external dependencies are ... this patch of using unsetopt BSD_ECHO really should've been accepted ages ago as workaround for broken local tests.

So I feel "punished", because now I'm stuck with broken tests (and a working Docker PR that went from good to broken). I'm stuck because any other "alternative" means lots of unnecessary work ... that won't even "close" this issue.

E.g. I don't know if you knew, but if you run ZSH in interactive mode and it doesn't find any dotfiles ...

... it prompts the user for setting up a new configuration!

This is the Z Shell configuration function for new users,
zsh-newuser-install.
You are seeing this message because you have no zsh startup files
(the files .zshenv, .zprofile, .zshrc, .zlogin in the directory
This is the Z Shell configuration function for new users,
zsh-newuser-install.
You are seeing this message because you have no zsh startup files
(the files .zshenv, .zprofile, .zshrc, .zlogin in the directory
/tmp).  This function can help you with a few settings that should
make your use of the shell easier.
You can:
(q)  Quit and do nothing.  The function will be run again next time.
(0)  Exit, creating the file /tmp/.zshrc containing just a comment.
     That will prevent this function being run again.
(1)  Continue to the main menu.
(2)  Populate your /tmp/.zshrc with the configuration recommended
     by the system administrator and exit (you will need to edit
     the file by hand, if so desired).
--- Type one of the keys in parentheses --- q

This probably doesn't matter if you're not running ZSH "truly" interactively. (e.g. as in Cucumber tests because bin/myscript is passed as a parameter to every script. It isn't "piped" or "talked to" through a pty/login shell. If Aruba every did "simulate more realistically" interaction with a shell, this issue would reappear AGAIN.

To avoid this, you'd have to "prepare" some default Zsh startup files, which means ... more copying statements, more files and more code in Aruba which shouldn't be there.

And who really needs the echo \c anyway? Why not just test the number? It's also not Zsh-specific, so it's a bad test anyway. If I can't recognize if the code is from Bash or Zsh, how does that test if Aruba actually used the right interpreter?

Even in Bash (where echo is not a built-in) I can do this:

function echo(){ /bin/echo -e "$@"; }

And now echo does what Zsh's echo can.

Might as well do something unique to Zsh and more "understandable" that magic with control characters, e.g.:

echo $((0.2 * 5))

On bash, it's a syntax error. On Zsh, it's 1..

So to wrap up:

1. ZDOTDIR isn't the perfect solution (and can actually be trickier)
2. echo -e isn't relevant to the test in the first place (and I don't see how it's "representative" of a user experience with a given shell), while unsetopt itself actually causes a nice syntax error on Zsh, so it's very relevant to testing if the right shell was used.
3. Alternatives are more work
4. There are probably nice and simple "one-liners" which are much better than the alternative.

And my question: what does it take to get a sufficient workaround for a broken test like this accepted immediately? It's neither a major feature nor a public relations disaster.

I get that new features need to be introduced carefully. But there's a difference between "deep logical/thought-provoking debate" and being so vague about reasons for not accepting, that it takes day s for accepting 4 lines of code in a cucumber feature.

I don't mind getting a point-by-point and "on point" list of what to fix to get this merge. (Or an outright immediate rejection with a clear reason). Having this stuck in the netherworld just makes me feel like going somewhere else to contribute. Because this way, I'm just stuck with a bottleneck I can't even deal with. (Or I'm given "options" that may just end up as time sinks or "neverending stories").

[maxmeyer]

@Huh, I'm impressed. Thanks for your explanation.

And my question: what does it take to get a sufficient workaround for a broken test like this accepted immediately? It's neither a major feature nor a public relations disaster.

Correct, it's none of those. I think it is a problem of assumption (on my side) vs. the real world (your setup) on the other side. I didn't have in mind, that writing the features in that way would break the test suite on some ones system.

Personally I try to balance if it's a general problem or more a problem of a specific setup. To make this decision I try to understand the reason behind the change.

I'm not a fan of using shims. If those tests are only for documentation, I would prefer to exclude them from a normal test run, because they've got only value for users, not aruba developers. And then let's run them on CI.

Suggestion: To get this particular problem fixed, let's remove exactly from the step where possible or rewrite the test. Then this should pass on bash and zsh. Did I understand that correctly and no unsetopt is required. This unsetopt just does not look right. Yes it fixes the test, but it's not really the problem as you pointed out before.

And who really needs the echo \c anyway? Why not just test the number? It's also not Zsh-specific, so it's a bad test anyway. If I can't recognize if the code is from Bash or Zsh, how does that test if Aruba actually used the right interpreter?

Yes, that code excels the shell's features not aruba's ones. I fixed that on master. But you're right there's still the problem with the external deps.

OFFTOPIC: Just for you to understand me better

I also try not merge things just to make people happy and then change things "behind" their back to "get my will". I don't think that's fair. I saw that happend to my code contributed to some other project on some other platform and that discourages me a bit because it looked like my code was not good enough, and the person was not happy with that, but didn't want to tell me. What I learned was, that OSS is much about communication, but I also that I'm responsible for the code contributed also the contributor has long gone.

I hope, I get this answer right. I think you deserve an answer to that response, but it's a little bit late for me not writing in my mother tongue. It was a long day.

[e2]

I didn't have in mind, that writing the features in that way would break the test suite on some ones system.

This is only a problem if there's no priority in fixing broken tests. Broken tests that no one is fixing is one thing, but if getting them fixed takes too much time, that's the most detrimental thing to people who are quality-oriented.

E.g. I don't mind "improving" and taking time to fix it. If something is an issue, you can always keep the issue open and quick-merge any immediate "fix".

In the "quality world", bugs are priority no #1. Whenever there's a bug, you "drop everything you're doing" and fix it. The most detrimental thing with the whole RVM issue is: it's broken because newer versions of RVM have a different output string, so the whole method just blows up. If there's no one interested in just supporting a new output string, it's best to just replace the error with a "won't fix anytime soon" message, and a "please consider releasing a gem with a fixed version if you need this - and here's the API you need to extend Aruba to get this fixed quickly.

If a project is a bottleneck preventing people from getting stuff done, you're forcing people to create their own forks and switch all their projects there - and now you have 2 forks. The "official one" and "one that moves faster". And the codebases divert quickly. Lots of frustration and wasted effort on both sides. All because ...

... fixing bugs wasn't a priority.

For me, getting PR count down to zero is always priority number one. Yesterday I just merged and released a "minor" patch - just because someone created a PR without a test (at first). Within a few hours.

From a contributor's perspective, if "some test" is failing locally, then every single next PR means the same failing test too. I don't like failing tests, but if I fix it, then either every next PR I have will include that same fix (for my tests to run), or I have to rebase and cherry-pick every PR to exclude my patch for this a the last step. I'm fine rebasing to master, and I can rebase to my own rebases (to prepare proper PRs), but that's a waste of time. I don't want to have 16 Aruba checkouts to switch between - all just to have 16 PRs I need to manage, reapply my fix after EVERY rebase to master in every PR, run 16 full tests of Aruba (and realizing I forgot my patch and that's the failure) ...

... it's a mess to work with, even if I have the skills to manage it.

If you can't respond quickly and decisively to a PR (no matter what the decision is - only you have write access anyway) ... then you're exponentially increasing the workload for anyone the more other PRs they have.

At this point I'm pretty much "maintaining my own fork" - just without having a sane remote master to work from. Thank goodness to Git that I can. (If I was using SVN, this experience would be utterly horrible).

Every PR is potentially a bug fixed that you didn't have to fix yourself. If you reject the bug fix and fix it yourself ... at least I don't have a failing master (on my system) so I can work on an improvement off of yours. Or an alternative PR.

In short: if you don't treat bug as an emergency (just like you would with security issues) ... you're just exponentially increasing the work of contributors for no reason. It doesn't affect your experience at all (or those of other users - aside from a handful who have just given up). But the contributor is stuck, can't move forward, and work is piling up. And it's getting worse with every PR or "discussion" to attempt to resolve an issue faster.

Branches don't help if you are the bottleneck. (Strong words, but that's reality). If you can't make decisions quickly, you are a bottleneck. If you can't make a decision whether to accept, reject or present specific "code-related" conditions for accepting, then at least decide not to make a decision (so I can just fork and move forward).

If you don't make quick decisions (whatever they are), it's a lose-lose situation.

I'm not a fan of using shims. If those tests are only for documentation, I would prefer to exclude them from a normal test run, because they've got only value for users, not aruba developers. And then let's run them on CI.

To someone outside, those tests are just "integration tests" for Aruba::ScriptFile. If you look at them that way, it's only natural to use shims. As a better testing practice.

I also try not merge things just to make people happy and then change things "behind" their back to "get my will".

You can always communicate the holdup clearly. Sometimes it just means "I don't know" or "can you explain why X?". Not everyone becomes happy through the same method.

Different things make different people happy.

E.g. "I won't accept shims because (specific X) and I won't accept the BSD_ECHO because (specific Y), but I will accept immediately a PR where the output is simplified with the 'echo "\c"' removed, even if that makes these tests technically useless and reduces coverage of Aruba::ScriptFile. Let me know if you want to fix this or if you'd like to just patch it up quickly.".

There. Just 65 words and I know exactly what my options are, what will happen, what to expect and you can even let me decide whether to "own the commit" or not.

I'd actually be happier if someone fixed the failing tests "behind my back". Because fixing other people's tests is not something that makes me super happy anyway. When all tests are green, I know my contribution will be accepted. When AppVeyor is failing, then every PR I have looks "broken" already.

Notice my first commit was actually functionally similar to yours - with the '-e' used to "force" it to work with minimal work.

The cause of this problem was: at some point, someone decided the echo "\c" was a good idea for an example. I don't know why you hesitated to remove that, and yet not accept the unsetopt (which to Zsh users may actually be a clear "acknowledgement" of their shell).

But you're right there's still the problem with the external deps.

And also: how this affects Aruba::ScriptFile coverage in integration tests.

E.g. you still don't know if Aruba isn't calling "bash" when users are expecting "zsh" (or vice-verse). (I think this kind of bug has popped up before, since "bash" is usually a user's default shell. Not to mention bash vs dash).

[e2]

What I learned was, that OSS is much about communication, but I also that I'm responsible for the code contributed also the contributor has long gone.

That's why as a maintainer you especially have to communicate if you're unsure about something and need more info. Asking a question doesn't mean you're a bad communicator. E.g. for me, "nice documentation" isn't a priority over "Aruba::ScriptFile coverage". Obviously if the unsetopt was ugly to you and the "\c" wasn't, (and "ZDOTDIR" was just another can of worms) I was lost as to what I could do quickly at least workaround my "failing tests problem". Disabling tests isn't a good idea. Even the Dockerfile patch just exploded into "more work" (instead of being accepted as a quick workaround for failing tests and improved later).

I'm a pro. What I don't like is completely wasting my time on irrelevant issues instead of making valuable improvements quickly.

So you probably have to decide whether you prefer to "avoid hurting feelings" or "discouraging pros".

Because you can't have both. If I create a PR in RSpec, I'm going to get a ton of very specific responses. Yes, it will likely take lots of time to cover all the points, but at any moment I know what I'm dealing with. It's also an extremely complex codebase (heavily based on introspection and an API countless projects grind away at daily). But, even then, every answer/question I get is quick and specific.

I hope, I get this answer right. I think you deserve an answer to that response, but it's a little bit late for me not writing in my mother tongue. It was a long day.

For me it's patch first - and you can reply whenever you have time (or not).

If I'm writing a lot, it just shows that the real obstacles to "work without waste" here ... are not something I can fix with code.