Unit Testing

[xcthulhu]

I've been following sente for a long time looking for an opportunity to deep dive into it.

So I decided to write a small suite of unit tests. Currently, in order to run my unit tests you have type lein devcards and visit http://localhost:3449/#!/taoensso.sente.client_tests

My next goals are to get these tests to work with doo and then get continuous integration with TravisCI or CircleCI up and running

[arichiardi]

Great! 👍

[danielcompton]

Could you move this section back to where it was in the previous version? It makes the diffs easier to read and understand.

[xcthulhu]

Done

[danielcompton]

Thanks heaps for this, really pleased to see this. I've added some comments.

[xcthulhu]

@danielcompton Okay, I've tried to take care of your comments.

I'm not sure that phantomjs testing is in the cards until #209 is resolved. On the other hand I have enough tests now that I might try my hand at a little brain surgery even though I'm still of a n00b to this code.

[ptaoussanis]

Hi Matthew, thank you so much for going to all this effort.

And I am really sorry to do this, but could I possibly ask that you pause further efforts until I've had an opportunity to take a look at your PR and comment?

Just handling an urgent project atm, but will try make this a priority when I can.

[xcthulhu]

And I am really sorry to do this, but could I possibly ask that you pause further efforts until I've had an opportunity to take a look at your PR and comment?

You mean go and do my day job for a while? I suppose if you insist.

Here's what left TODO: finish doo testing ; I think I can get around #209 with karma, but I'm not sure.

If all you want is devcards testing we can leave it at that. Here's a mini tutorial for how to use it:

1. Type at the command line # lein devcards
2. Wait for it to say Prompt will show when Figwheel connects to your application
3. Go to http://localhost:3449/#!/taoensso.sente.client_tests in your browser

Everything should be green and there should be no red (red means the test failed).

We can leave it at this for the first cut if you want; but I'd want to get rid of the doo stubs.

[ptaoussanis]

Hi Matthew,

Thanks for being so patient with me on this; have had my hands very full recently + wanted to take the time to properly explain my rationale re: this PR.

While I really (really!) appreciate the effort you put into this, I'd actually prefer not to merge tests like this into Sente. To be clear: there's nothing wrong about your tests in particular, would just prefer not to have tests like this in principle.

A few factors I have in mind here:

\1. Something like Sente has a relatively small public-API surface area. Most of what could go wrong with Sente is either implementation-level stuff or logical stuff.

Logical stuff isn't helped much by unit tests. And Sente's implementation-level stuff is either not very interesting to test, easier to verify with something like Truss, or requires tight coupling between the implementation and tests (implying a significant + ongoing test maintenance effort).

\2. The trade above might be worth it if Sente relied a lot on PRs since tests would maybe let users not too familiar with the codebase make changes with at least some confidence that they're not breaking things.

This isn't the case with Sente though. I use it heavily myself, and will continue to personally vet all core changes that come through.

My time's really limited lately, so just need to be pretty realistic+aggressive about where I can best spend it. My estimation is that it'd realistically take less effort for me to carefully look at the odd PR than to maintain a whole set of unit tests that I wouldn't personally benefit much from relatively to their maintenance cost.

\3. Most of Sente's core functionality can actually be tested reasonably well just through observing the reference example. It exercises a number of core features like connections, fallbacks, async push, event buffering, un/packing, callbacks, etc.

Any major implementation or logical issues will generally mean that something obvious in the ref. example breaks.

So in fact we already do have a maintenance-burden-free way of checking that the basics are all working. And, indeed, even if we had unit tests- it'd still be necessary to check major changes against the ref. example since that lets us test concurrency, different browsers, mobile browsers, etc. in a way that's impractical to do through unit tests.

Historically, the bugs that've found their way into Sente in production have typically been the kind of edge-case logical issues that unit tests wouldn't have caught anyway.

\4. ClojureScript-side unit testing isn't quite as mature as I'd like yet. For one, there's still more tools/config churn than I'd like to see. This unfortunately just adds incidental fluff to the maintenance costs.

---

If the situation ever changes (e.g. I get hit by a bus or otherwise stop maintaining Sente) - it might be reasonable to reexamine the idea of adding unit tests. Or (possibly a better idea): just expanding the ref. example to make sure it exercises everything it can + to better report any unexpected behaviour.

If one wanted something fully automatic, an ideal unit test might consist of an expanded ref. example + some scripts to automatically interact with it and report back on any unexpected behaviour.

That'd have the added benefit of helping to exercise more of the most interesting sources of potential problems (different client platforms, different network environments, etc.).

In the meantime (i.e. while I'm still personally involved), it just makes more sense for me to avoid any kind of maintenance burdens that won't be a clear net win in practice - even if that means being a bit of a nuisance for potential PR submitters.

I hope that makes sense / seems reasonable?

Again, really do appreciate how much time+effort probably went into this work - am not declining this lightly.

[xcthulhu]

I think you're being a bit hasty here...

Are you sure you wouldn't profit greatly from setting up CI?

From what I can tell, you have an enormous support matrix:

Protocols:

1. WS (+ over TLS)
2. Long polling (+ over TLS)

And proposed:

1. WS with binary transmission
2. Long polling with binary transmission

You also have all of these clients:

1. Nashorn
2. Chrome/V8
3. Firefox/SpiderMonkey
4. Safari/JavaScriptCore
5. IE/Chakra

And proposed:

1. Node.js/V8
2. Clojure

And all of these servers:

1. Node.js
2. http-kit
3. immutant
4. nginx

And proposed:

1. Aleph

And all of these serialization formats

1. Transit
2. Nippy
3. EDN

Not to mention supporting everything with advanced compilation...

...I'm undoubtedly a novice engineer compared to you, but I cannot fathom how you can maintain support for everything without any automated testing. What happens if an upstream dependency makes a breaking change?

[danielcompton]

@ptaoussanis if the tests were higher level, and only tested the public API, would you be more happy with that? They would basically be integration tests, not unit tests.

[ptaoussanis]

@xcthulhu Hi Matthew, I do appreciate that my perspective on this might not make a lot of sense to you. Not a hasty decision though, I do promise :-) For the moment, just really do need to be aggressive about how I prioritise my time. For the reasons I described above, don't think this offers competitive bang/buck at the moment.

As it is, have 20k+ lines of Clojure that I'd like to try find the time to publish but that I haven't had the opportunity to yet.

Looking over this PR, I'd need at least a few hours to clean up tests that are testing things we shouldn't be testing (implementation details or stuff that should be tested upstream); or to add tests for things we should be testing if we've got tests that people are going to try rely on. Having misleading unit tests can be harmful.

As I described above; ideally, I'd prefer tests that simulate+exercise a robust test example + scripts to observe for any unexpected public-API-level behaviour. But that'd be quite an undertaking and just not a priority right now. Like I say, for the moment, I'd prefer to just carefully think through + vet changes myself.

From what I can tell, you have an enormous support matrix

Just to clarify: Sente's protocols are pretty simple and have clear contractual requirements. If an upstream server, say, is buggy (doesn't adhere to its own published API contract), then that's something that the upstream server's tests should catch. Could we try catch these things Sente's side? Sure; but again- it's a tradeoff. We could also try test for unexpected Clojure behaviour or even OS behaviour; but at some point one needs to draw a line and say: here's the contract, our responsibilities are w/in this scope. Where that line goes is a tradeoff.

If folks would like to write tests for specific adapters, etc. - I'd be very happy to link to those. Just don't want to pick up stuff right now that I'll personally need to maintain unless it's a clear win. I really just don't have the time; spending time here will mean I don't get to publish other code that I think many more people might find much more useful.

but I cannot fathom how you can maintain support for everything without any automated testing.

As I explained above (maybe too briefly, sorry)- automated testing can be helpful, but has limits and costs. In this very particular case, the limits are significant and the costs high. Until the economics of the trade change, I'd rather not incur the significant maintenance burden for what'd be of negligible benefit to me (personally). Again, I understand and regret that that might be a little hostile to outside contributors.

Just need to work with the time I have, can't manufacture any more. Need to spend it where it's practically going to have the biggest payoffs, even if that means the occasional bug gets through that may have been caught by an appropriate unit test. In fact- these bugs are usually rare, usually small, and usually wouldn't be caught by these kinds of unit tests anyway.

It may be instructive to examine the 2 most recent bugs in Sente core- #230 and #201. Both of these were caused by logical errors re: edge-case spec behaviour I wasn't aware of. Not even sophisticated unit tests would have helped in either case, because we wouldn't have known to be looking for these issues.

@danielcompton Hi Daniel, that's a very fair question. Honestly, yes. But getting a set of tests from these that I'd be happy to merge would take a significant amount of time (both mine and the submitter's). Realistically, it'd make sense for me to just write the tests if/when the economics ever change to make this a clear net win.

---

@xcthulhu
Going to be closing this for now since I won't have time to follow up the discussion any more; going to be travelling soon. Really really do appreciate all your work + feedback on this (and your other fantastic contributions to Sente). I hope my perspective on this might seem rational, even if you disagree with some conclusions :-)

All the best, cheers!