End-to-end tests

[johananl]

At the moment we have no e2e tests for Clisso. It would be extremely valuable to be able to verify the application actually works, possibly on every pull request etc.

I was thinking about adding a directory for e2e tests, and using the built-in Golang testing library to spawn a Clisso process with various arguments, then verifying the process' stdout and result (credentials written to shell / file etc.).

Ideally we should have a way of doing so without communicating with real resources. The OneLogin API can probably be mocked easily using a simple HTTP server with pre-configured responses. Mocking AWS IAM may be slightly more complicated (but maybe something like this or this could help?).

[johananl]

Can get inspiration from https://github.com/hashicorp/terraform/blob/master/command/e2etest/primary_test.go.

At the moment, Clisso can only work in interactive mode. This isn't ideal for testing (because we have to use Expect-like functionality for e2e tests), and it also doesn't allow Clisso to easily be used in scripts.

A possible solution is to allow specifying all user input as CLI arguments, and prompting the user only when a required input is missing (similar to how Terraform behaves with variables). Off the top of my head, this includes only username, password and OTP. Things like selecting an MFA device from a list can simply be skipped in tests as long as we test tricky areas in unit tests.

Side note on allowing password as a parameter: This will stored in shell history and poses a security thread. Currently if you have added the password to the system keychain, stored the username in the config file and only have one MFA device no input is needed on the shell. For testing we would need to additionally mock the Linux password storage and would be good to go.

[lahavsavir]

I agree that Password as a parameter is something we should avoid. Lahav

…

-- *Kind Regards,* [image: allcloud-signature-icon-3.png] Lahav Savir Founder, EVP and Chief Architect AllCloud, Cloud Platforms m: +972 (54) 4321688 w: www.allcloud.io e: lahav.savir@allcloud.io Thoughts on our interaction? *Provide feedback here <https://www.surveymonkey.com/r/allcloud-nps?email=lahav.savir@allcloud.io>*

On Fri, Jan 4, 2019 at 11:08 AM Jonathan ***@***.***> wrote: Side note on allowing password as a parameter: This will stored in shell history and poses a security thread. Currently if you have added the password to the system keychain, stored the username in the config file and only have one MFA device no input is needed on the shell. For testing we would need to additionally mock the Linux password storage and would be good to go. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#71 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEFW6MCpZdEvVromWpSlpf9hKPMZC1woks5u_xoBgaJpZM4Wlqji> .

-- This message and the information contained herein is proprietary and confidential and subject to the AllCloud policy statement, you may review it here <https://bit.ly/2Mu90e4>.

[johananl]

Sure, having clear-text passwords in the history is definitely a bad idea. I'm not suggesting an actual design change, my main point is that we need tests. In the current state we have no confidence that we didn't break things when making a code change, and we've already had cases where we accidentally broke stuff when introducing something new. The codebase is starting to grow and we must have tests in place.
In addition, there is a lot of refactoring that I'd love to do, but I can't do that in confidence without tests covering the important areas.

[lahavsavir]

Got it. How do then suggest to have this feature? undocumented? or --unsecure-password=xxx Lahav

…

-- *Kind Regards,* [image: allcloud-signature-icon-3.png] Lahav Savir Founder, EVP and Chief Architect AllCloud, Cloud Platforms m: +972 (54) 4321688 w: www.allcloud.io e: lahav.savir@allcloud.io Thoughts on our interaction? *Provide feedback here <https://www.surveymonkey.com/r/allcloud-nps?email=lahav.savir@allcloud.io>*

On Fri, Jan 4, 2019 at 1:15 PM Johannes Liebermann ***@***.***> wrote: Sure, having clear-text passwords in the history is definitely a bad idea. I'm not suggesting an actual design change, my main point is that we need tests. In the current state we have no confidence that we didn't break things when making a code change. The codebase is starting to grow and we must have tests in place. In addition, there is a lot of refactoring that I'd love to do, but I can't do that in confidence without tests covering the important areas. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#71 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEFW6IJ6oGwbK1I2u_C-gB-kOBUZFqXhks5u_zfngaJpZM4Wlqji> .

-- This message and the information contained herein is proprietary and confidential and subject to the AllCloud policy statement, you may review it here <https://bit.ly/2Mu90e4>.

[johananl]

I'm not sure yet. My main goal is to make e2e tests possible for Clisso. Ideally, it should be possible to test a CLI tool by simply calling the binary with a fixed set of arguments and verify the output against a known file or so. We could go a different route and actually "type" things interactively like a user using goexpect for example, but this kind of testing is much more fragile.
I'll look into it a bit more and look at similar projects and how they've addressed the problem.