bramstein / knockout.selection

A selection binding for Knockout.js
19 stars 9 forks source link

Test that the cleanup on init actually works as expected #73

Closed mwoc closed 10 years ago

mwoc commented 11 years ago

Until now, we didn't test that the subscriptions cleaning up the 'selected' property on items removed from the selection observableArray were actually called. This pull request fixes that, a test was added too (added extra devDependency sinon)

mwoc commented 11 years ago

Seems the new test doesn't run as expected on IE8 and IE9, though I can see that IE8 does do the cleanup correctly, it just happens after the test suite does the expect() calls. Will report back about that when I get the time

mwoc commented 10 years ago

@sunesimonsen - I've based the testing of code that is inside setTimeout callbacks on how sinon is implemented in knockout.list - whose tests also don't run properly in IE8 (haven't got IE9 available currently). Do you happen to know some alternative way to test the behaviour which also works in those older browsers?

@bramstein - if tests for code run in setTimeout callbacks can't be run properly in IE8/9, would you consider dropping CI (or specific tests) for those browsers, or rather not use features which depend on setTimeout, such as the one this pull request is for?

bramstein commented 10 years ago

@mwoc Could you explain in a bit more detail what exactly the issue in IE8/9 is? I'm not sure I fully understand the problem.

mwoc commented 10 years ago

@bramstein - yes, I've tried using sinon fake timers in the tests, to test the one line of code which is executed after a setTimeout. These tests run fine on all browsers, except IE8 and 9. After some further tinkering I however figured one could just use a real timeout inside the test, rather than sinon's fake timeout, and then call the '''done''' callback.

Am waiting for Travis CI to see if that way of testing works nicely. If it does, the PR becomes a bit easier as we won't need Sinon. So keeping my fingers crossed..

mwoc commented 10 years ago

Ah there we go :) The comments from before the last two commits don't apply anymore, it's safe to merge this in. The diff is a bit cleaner now too.

bramstein commented 10 years ago

Nice work! Merging this in. Thanks!