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

Test sup command #135

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Test sup command #135

wants to merge 12 commits into from

Conversation

mingan
Copy link
Contributor

@mingan mingan commented Mar 25, 2018

This PR builds on top of #134. It adds tests for sup commad as a sanity check of the existing functionality. In order to accomplish it I had to adjust the code in few places:

  1. I moved most of the code from main to runSupfile so that it can be run from tests
  2. Most flags are parsed into a struct so that tests don't depend on global vars (and it's a struct instead of eight separate variables)
  3. To keep the test output clean the error output stream is passed around explicitly and set to ioutil.Discard in tests
  4. SSHClient got new fields so that I could remove the global variable initAuthMethodOnce preventing running the command multiple times (aka when running tests)

The tests which need to spin up mock SSH servers, capture the requests and always respond with success. I barely understand this part and it's put together from different examples around the web.

I'm doing this as a preparation for tackling #130.

@mingan mingan force-pushed the 130-reuse branch 3 times, most recently from c54ab0e to e86a6ec Compare April 15, 2018 11:40
@mingan
Copy link
Contributor Author

mingan commented Apr 15, 2018

Updated the test setup to get it running on Travis CI.

Copy link
Collaborator

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome.

But I'm too busy and don't have time for a thorough review right now. I'll come back to this eventually

Copy link
Collaborator

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain in high level what the provided tests do exactly?

@mingan
Copy link
Contributor Author

mingan commented Apr 18, 2018

Sure, the basic idea is to move the most of the main function to a separate function. main now just gathers flags, sets up dependencies and calls this function. Tests do the same. Normal de-coupling from flags, global vars etc.

Each test spins up mock a set of SSH servers which only record command and provide them back for assertions, they don't do anything else. The server addresses are written into an SSH config file in a tmp directory alongside generated key and cert for the SSH servers, public and private key pair of the client.

Every test contains YAML string with the Supfile being tested. The test runs this Supfile from parsing YAML, to setting up App and actually performing the commands. matcher_test.go provides convenience functions for testing that commands were sent to the defined servers and can test individual exports or commands. It's based on comparing the requests sent to the servers as strings, nothing more.

The tests cover basic things like missing networks, commands, targets etc., simple command, sequence of commands, a target, --onlyHosts, --exceptHosts, hosts specified via inventory, default SUP_* variables, setting global, network and command line variables as well as custom file name.

The code for the mock servers is horrible. I assume it can be improved but I just stitched it together from different examples and by trial and error.

I'm happy to answer any more questions.

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

Successfully merging this pull request may close these issues.

2 participants