Parameterize connection strings in integration tests. Set up testing for AppVeyor and Travis

[eiriktsarpalis]

This PR parameterizes connection string inputs for the integration tests. This allows us to run the same test suite both with the storage emulator and an actual storage account. From FAKE,

./build.sh -ef RunEmulatorTests -ef RunRemoteTests

In order to get the latter category up and running, the environment variable FSharpAzureStorageConnectionString must be set to a working azure storage connection string. This can be then used to set up testing in both AppVeyor and Travis. I have also included travis and appveyor configuration files in the repo.

[daniel-chambers]

I'm interested in your reasoning about why "Emulator" and "Remote" tests are considered separate sets of tests that can both be run together, in comparison to simply parameterizing the connection string, with the default being the emulator (for ease of local dev) and it being overridden in the Appveyor/Travis build scripts with an actual storage account (because of the lack of an emulator in those build environments).

Seems like it would be simpler to just set (override) the connection string and run the tests, instead of having to set it and then having to explicitly choose to run remote tests?

I'm happy to set up Appveyor and Travis builds to utilize your good work here; we can hook them up to run off a storage account in my Azure subscription. However, I suspect we could only get it to work for the master branch, not for PRs, because enabling it for PRs would effectively expose the storage account connection string publically.

[eiriktsarpalis]

It should be possible if XUnit permits passing arguments to the console runner. My decision to use inheritance in this case is mostly influenced by my familiarity with NUnit. If there is a more idiomatic way to express this in XUnit, please let me know.

As far as AppVeyor and Travis are concerned, you can simply set the connection string as an environment variable in the admin panel. I think that this works nicely with PRs, without exposing that information.

[eiriktsarpalis]

Also, I think it is entirely reasonable to expect that the dev might want to test against both the emulator and an actual storage service. Often when working with mbrace on azure we have discovered minute differences in behaviour between the two. The emulator is for convenience, but ultimately you need to ensure that it works with azure proper.

[daniel-chambers]

Good point, testing both is useful. I'd like to keep the concept of the differing environments (aka connection strings) outside the tests though. How about the below approach:

In order to pass parameters to an xunit test, we can set the environment variable controlling the connection string as a process-level env var in the fake script. Then, when the xunit runner is started, it inherits the env var from the parent process (fake). That way we can simply execute the same tests in different contexts just by varying the env var in fake. Plus, because the env var is a process-level env var, it disappears when fake quits, so we're not polluting the global environment.

I've made some modifications to your changes here to demo the idea: https://github.com/daniel-chambers/FSharp.Azure.Storage/commit/5b8809d12fd733ca59bc76a03f7bde1ae6543bfb

With respect to builds, it's pretty easy to exploit the PR system to expose environment variables set in a build system. All an attacker needs to do is just write some code that takes the env var value and outputs it to the console, or sends it to a web service, or whatever, and then open a PR with that code in it. Sure, I (as the reviewer) might notice the attacker's malicious code, but there's a period of time where an attacker has captured my storage account connection string and can go nuts against it. :) While I trust the F# community to not misuse it, this repo and PR system is open to the public Internet.

[daniel-chambers]

As a reference, AppVeyor's doco on secure variables notes that they don't decrypt and provide them for PR builds, because of the risk of malicious PRs from the public internet.

[eiriktsarpalis]

But doesn't this also mean that you would have to run separate builds to test both cases?

[eiriktsarpalis]

Good point on PR security btw. As far as mbrace is concerned, I think it's a risk I can bear to live with.

[daniel-chambers]

Agreed, running two builds would be annoying. We can simply run the tests twice in the same build, once with the storage emulator connection string, and then again with the remote connection string. See in the RunTests target we call runTests twice, modifying the env var each time.

[eiriktsarpalis]

Sure, but I would imagine that specifying test categories is more idiomatic than mutating env vars during the build.

[daniel-chambers]

It's the same tests effectively copied into multiple categories by using inheritance. Each fixture needs to be explicitly specialized to run in both environments where the only difference is the connection string; the tests are identical.

If this was regular, non-test, code, conceptually I think we'd rather call a single function twice with a different argument value, instead of creating two subclasses that each have a different parameter value baked into the constructor, then instantiate each instance and call it. On 13 Jan 2016 12:51 AM, "Eirik Tsarpalis" notifications@github.com wrote:

Sure, but I would imagine that specifying test categories is more idiomatic than mutating env vars during the build.

— Reply to this email directly or view it on GitHub https://github.com/fsprojects/FSharp.Azure.Storage/pull/18#issuecomment-170917387 .

[eiriktsarpalis]

Yes, this is based on a commonly used pattern (see 1, 2 and 3). While the underlying test suite is indeed identical, this is not to say that the system under test remains the same in emulated and non-emulated storage. We rely heavily on this pattern to deliver a runtime agnostic test suite in mbrace for instance.

One could argue that this approach is not in tune with a functional programming style, however it is forced on us by constraints of the test runner engine. A functional-first test library like fuchu would allow us to be more elegant in this context.

It must be noted that AppVeyor does not use FAKE when running the tests. This means that we can only run one set of tests per build, but not both, assuming we also wanted to set up a storage emulator there.

Finally, when working with FAKE/travis we would need to run a separate xunit test process for each of the categories. This essentially means that we would be getting a partial test report in case of failing tests in both categories (FAKE raises an exception when an xunit process completes with failures, effectively halting the build script prematurely).

[daniel-chambers]

Fair enough, you make a good argument. Let's go with your approach.

I don't think we should bother trying to get storage emulator tests running in Appveyor. I think testing against a real storage account is good enough.

[isaacabraham]

Only thing I would say about a real storage account for tests - how are you managing the key to it (injected by AppVeyor?) and are there any costs you need to be aware of with respect to azure storage?

For the TP we use the local dev emulator and didn't run into problems, but then again we don't have a CI build for it.