Changes to allow rake test to work on Windows; set up Windows-based CI

[mattbrictson]

This PR makes the airbrussh tests work on Windows and sets up a Windows-based CI. The build status can be found here: https://ci.appveyor.com/project/mattbrictson/airbrussh

Getting airbrussh's tests to execute on Windows was a bit of a challenge, mainly due to the acceptance tests assuming a UNIX-like environment. I had to make the following fixes:

1. AppVeyor (the CI service) apparently does not use a pty, so $stdout.tty? returns false in that environment. This had the effect of disabling color in the Pretty log file output. I worked around this by setting SSHKIT_COLOR=1 during the tests to force color output.
2. There is no such thing as /usr/bin/env on Windows, which by default SSHKit prepends to every command. To fix this, I faked the SSHKit command map to never prefix commands.
3. Env.getpwuid (used by SSHKit::Host) doesn't work on Windows. I stubbed it.
4. [ -f ~ ] is not understood shell syntax on Windows. I replaced it with echo.

All tests now pass on both Travis and AppVeyor.

@robd In your opinion have a mangled the acceptance tests with these changes? I am particularly worried that by using the SSHKIT_COLOR=1 and command map faking that the tests have diverged slightly from the typical SSHKit configuration, and therefore the tests are now of lower quality.

Windows compatibility is not a high priority, so if the acceptance tests have been compromised as a result of my changes, perhaps it is not worth merging this. Let me know what you think.

[robd]

Hey @mattbrictson

Sorry for the delay in getting back to you on this. This is very interesting to see what is required to get this to work on windows. I guess my overwhelming feeling is that it reinforces the need for acceptance tests against NetSSH on to Linux (which I imagine is more common) rather than against local windows backend.

Re removing the default command map, I think this is probably reasonable. There was already some hackery to account for the difference between linux and osx. Since the prefix (if present) is formatted the same as the command, I think we don't lose much coverage by removing it across all tests. If the SSHKit readme is right, we may just be able to assign the new map without stubbing:

SSHKit.config.command_map = Hash.new do |hash, command|
  hash[command] = command.to_s
end

Re the SSHKIT_COLOR=1, I tend to agree that this causes the tests to move away from the normal behaviour we would expect in all environments (windows, osx, linux) against SSHKit master (ie no colors). I guess that the windows tests only failed against old (ie 1.7.1 and before) versions of SSHKit, because master doesn't expect output colors.

I may have misunderstood something here, but one thought I had here is, rather than changing the output to force colors, whether we could change the assertion not to expect colors on windows?

The assertion changes to allow > 1 second commands are improvements.

I am a bit confused about this:

Etc.stubs(:getpwuid => stub(:name => "user"))

because the name is hard coded to "user", but that is never asserted on in the tests (where the user name is 'test_user'). I guess the result of getpwuid never actually ends up being used? Maybe this is a bug in SSHKit - why is it calling getpwuid if it is not used. In any case, maybe we could just stub this without a return value? (I haven't tested this):

Etc.stubs(:getpwuid) 

Re changing the test commands to use echo which exists on windows, I think this is probably OK. The only reason why I added the test commands is that I couldn't remember how they worked in historic SSHKit. In master, they are straightforward wrappers for execute.

It's finely balanced, but I think if we can remove forcing colors in the log output (SSHKIT_COLOR=1), this PR is worth merging. I imagine testing on windows may catch some things which we wouldn't otherwise even consider. I certainly am not running on windows at all.

Hope this is of some help - let me know your thoughts.

[mattbrictson]

@robd Thanks for the review.

If the SSHKit readme is right, we may just be able to assign the new map without stubbing

You're probably right, but since SSHKit.config.command_map is global that would potentially pollute other tests. To be safe, I'd have to store the original command map and then revert it using teardown. I like the stub approach because it accomplishes the same thing but with less code.

rather than changing the output to force colors [with SSHKIT_COLOR=1], whether we could change the assertion not to expect colors on windows?

Good idea. I will make that change. It seems like it should be straightforward since there is already conditional logic for color.

Etc.stubs(:getpwuid => stub(:name => "user"))

This is because SSHKit's host.rb sets its @user value by calling Etc.getpwuid.name when the host is :local. On Windows this blows up because nil does not respond to name. So technically this is an SSHKit bug.

[robd]

OK, I just lost a massive comment that I wrote here (thanks Github), so apologies if this is a bit brief!

I'd have to store the original command map and then revert it using teardown

I think this is already achieved by the call to SSHKit.reset_configuration! which is needed to teardown the sshkit config set up in the configure methods. I think it would be better to set up all elements of the config the same way for all config properties like this: robd@9fa384f

Etc.stubs(:getpwuid => stub(:name => "user"))

The thing I think is confusing here is that the name stubbed is 'user' but the name asserted on in all of the tests is 'test_user'. Obviously the stubbed name isn't actually used because of this line, but I think this makes the intent of the stub confusing. Any stub would work here eg: Etc.stubs(:getpwuid => stub(:name)), Etc.stubs(:getpwuid => stub(:name => "UNUSED")).

In fact I think the correct thing to do here is to alter the test to only stub @user on the host for SSHKit 1.6.1. This means that the return value from getpwuid is now used on SSHKit >= 1.7.1. This in turn means all the tests fail with the wrong username as I would expect:

Expected / ✔ 01 test_user@localhost \d.\d+s\n/ to === " ✔ 01 user@localhost 0.003s\n".

So the getpwuid stub must be fixed to the correct name Etc.stubs(:getpwuid => stub(:name => @user))

Like this: robd@2599b1c

[mattbrictson]

@robd Thanks again for the detailed feedback. I've rebuilt this PR from scratch with cleaned-up commits, and incorporated the changes you suggested.

Specifically:

• Rather than set SSHKIT_COLOR=1, the test now expects color or non-color output based on the build environment ($stdout.tty?) and the SSHKit version (> 1.7.1 is non-color).
• Explicitly set command_map rather than using a stub, and do so within the existing configure method alongside other configuration code.
• Stub the username as Etc.stubs(:getpwuid => stub(:name => @user)) and consistently use @user when building assertions.

I think this is now ready to merge, if you agree!

[robd]

@mattbrictson Sorry for the delay here. I just looked through the latest changes and this looks good to me.