Functional tests

[jerith]

This is the start of my attempt at building a suite of functional tests for slacko. It's nowhere near ready to merge, but it's a reasonable starting point for a conversation about what we'd like this to become.

The only change to the slacko code itself is that base_url is now a reference and a new set_base_url function (which I've told ocamldoc to leave out) allows it to be set externally.

I haven't quite figured out how to handle the tension between strict test cases that make use of known data and the much more relaxed tests that can be run against the real API to make sure slacko continues to work properly as the slack API evolves over time.

For now, I plan to add simple tests around a few more functions and maybe figure out how to generate test coverage reports (coverage is done).

Here are some problems the tests have already uncovered:

• Channel lookup turns all sorts of errors into Channel_not_found, masking legitimate responses like Invalid_auth.
• The channel object returned by channels.create contains "0000000000.000000" in the last_read field while we expect a timestamp/float. I don't know if this is a special case or if this field is always string-encoded like this -- channels.list doesn't return that field.

I'd prefer to address these problems in separate branches and keep this one focused on the tests themselves, but it would probably make sense to merge or rebase some of the fixes back into this branch.

[jerith]

The coverage thing turned out to be pretty easy. I enabled coveralls for my fork (see https://coveralls.io/github/jerith/slacko for the results) but it'll have to be enabled separately for the main repo.

Adding test runs to the builds seems to make them significantly slower. :-(

[jerith]

These tests would be much easier to work with if there were a good way of turning errors back into strings (which there is already an issue for: #4).

I'm starting to uncover some bugs with these tests. I don't really want to fix them in this branch, because the focus here is on writing tests for the existing code, so I'll make a note of them in the PR description above (where they'll be more visible than buried in the comments) so they can be addressed separately.

[Leonidas-from-XIV]

I think this is a very promising approach and apart from the mutable bit that I'd like to avoid it looks very nice. Looking forward to your input.

[Leonidas-from-XIV]

I think we whould turn the module into a functor and split it into a inner module Config (though not sure what to really name it) module which contains stuff like base_url and an instantiation of the module with the default config module applied, so users will retain their API while we make parts modular and the test can therefore instantiate the functior with a different Config.

The advantages I see here is that this inner module can also hold other information like a cache of username/channel etc to it's ID and would be localized to a base_url. This is why I am not super sure with the name Config as it could be used for other useful things which do not map to a config.

Let me know what you think.

[jerith]

I don't really like the thought of tying state to a module, because it makes it harder to (for example) talk to multiple slacks with different tokens.

What about something like this:

type session = {
  base_url : string;
  token : token;
  mutable cache : some_suitable_type; (* maybe the value itself can be mutable instead *)
}

let create_session ?(base_url="https://slack.com/api/") token = {
  base_url;
  token = token_of_string token;
  cache = empty_cache; (* or something similar *)
}

We make the session type abstract and we use it everywhere we're currently using token. We can also turn token_of_string into a wrapper that creates a new session and keep the external API the same for everything except functions that talk to slack and don't currently need a token (which I believe is only api_test and oauth_access).

I'm happy to prototype both this and the functor version you suggested (in separate branches) and see which one works better.

[jerith]

I hacked up a proof of concept for each version.

Functor

Branch: https://github.com/jerith/slacko/tree/functor-based
Comparison to master: master...jerith:functor-based

This doesn't change anything in the existing public API (except to add stuff) but it shifts an awful lot of code around. (I didn't reindent the module signature or the code in the functor, otherwise the diff would be rather bigger.)

Choosing whether to use the Slacko module or a custom-built version wrapping a different base_url at runtime got a bit awkward, so I just made a copy of slack_notify that takes a mandatory --base-url parameter as an example of the functor's use.

Session value

Branch: https://github.com/jerith/slacko/tree/session-based
Comparison to master: master...jerith:session-based

This changes the signature of all functions that take a token and has them take a session instead. Like token, session is an abstract type. token_of_string now returns a session instead, but you need to use make_session if you want to (optionally) provide a base_url value. The two API functions that didn't take a token (api_test and oauth_access) now accept an optional ?base_url parameter.

I added a --base-url option (which does the obvious thing) to slack_notify to test the session stuff.

Thoughts

I compiled my slackobot code (unmodified) against both versions to verify backwards compatibility (at least for the parts of the API I'm using). I also modified my code to use the session value version explicitly (which was very straightforward) and attempted to do the same for the functor version, but I had forgotten to export the Sig module and thus couldn't use the functor. The my_slack_notify code gave me a feel for using that anyway, so I didn't bother fixing the problem.

While I prefer the session value version, I'm happy to take whichever you're happier with and open a pull request with a cleaned up implementation, etc.

[Leonidas-from-XIV]

Thank you for the comprehensive comparison. I have thought a bit about this and agree with your conclusion.

I do not mind as much breaking the API (we are still 0.x so figuring out a decent API is completely okay) nor the code changes in the functor approach but it's true that the functor does not really bring any advantage from the perspective of types, it's just more of a hassle to be able to use it.

[Leonidas-from-XIV]

With an inner module, we might not even need to have a function like this. Though ocamldoc probably will output documentation for the unapplied functor, which regular users should not need to care about.

[Leonidas-from-XIV]

Why do you need to extract values from these types? To do comparisons? I feel this is a bit brittle, as the types now have to be declared effectively twice. Correct me if I am misunderstanding.

Another idea: adding comparison functions to Slacko which compare some (relevant) values of the records without exposing the inner workings.

[jerith]

The tests need to build expected values to compare the actual values to. I was initially just using channel_of_string and user_of_string, but both of those explode on empty strings and there are places in the API the return empty strings for user fields.

This abbrtypes thing definitely needs to be replaced with something better -- it's very much a temporary hack to let me move on with the more useful test code without having to solve this problem properly first. :-)

[Leonidas-from-XIV]

Picky, but I'd rather like this to have a proper name.

[Leonidas-from-XIV]

I'd be nice to have these magic constants factored out with understandable names like valid_test_token in this case, so the reader knows what this branch represents.

[jerith]

fake_slack is currently very specific to the test data it's working with. It probably needs to become more general, but that ties into the difference between unit/functional/integration tests and how we handle the much less predictable data we get when talking to the real slack API.

[Leonidas-from-XIV]

Same here, general_channel, regular_channel and archived_channel come to mind.

[Leonidas-from-XIV]

I'd just match on exception. I think we do that somewhere anyway (and depend on a recent enough OCaml version) and I think it looks nice and coherent.

[Leonidas-from-XIV]

These notes are a nice idea, though should probably go on stderr.

[jerith]

They should, thanks.

[Leonidas-from-XIV]

I like how it uses an already provided printer. Printing things can be so painful sometimes.

[Leonidas-from-XIV]

Turns out the current cohttp depends on a version of conduit we can use.

[Leonidas-from-XIV]

cohttp 0.21.1 (released only a few days ago) has a dependency on conduit ≥ 0.14.0, so I suggest bumping that dependency.

[Leonidas-from-XIV]

Glad to see progress on this and I think it goes in the right diretion.

How about this: let's get this into a "releaseable" shape, since the last release is very old and publish it. Then iterate on writing tests and fixing what is broken. The downside is that the new release might work worse than the old version, which assumed less of what Slack was returning.

[Leonidas-from-XIV]

Can you put this comment above the line? This prevents lines from getting too unwieldy long and ugly.

[Leonidas-from-XIV]

Here's an idea: instead of specifying all JSON, maybe it would be possible to write a minimal stub which can generate a fake Slack endpoint by looking up <endpoint_name>.json in tests/ and returning that.

[jerith]

There isn't really enough commonality between the different endpoints to make that work easily. For some of them (such as the history APIs), we'll also need some more complex logic to decide what to return.

I'm hoping to eventually replace this with a more generic test double that doesn't rely on a bunch of stored data, but that's too big for this PR.

[Leonidas-from-XIV]

I would prefer |> to be in it's own line, like in slack.ml.

[jerith]

How about this: let's get this into a "releaseable" shape, since the last release is very old and publish it. Then iterate on writing tests and fixing what is broken.

That's pretty much where I was headed too. I'd like to at least cover the issues from #10 before merging, but that shouldn't take too much work.

[jerith]

#18, #19, and #20 fix all the problems that currently require tests to be skipped in this branch.

Is there anything else you'd particularly like tests for in this branch? chat_post_message is the only thing I think really needs to be tested before a release, but I'm happy to do that in a new PR after this one is merged.