disabling cross-instance storage test, node tcp pause/resume test

freedomjs / freedom

Embracing a distributed web

http://freedomjs.org

Apache License 2.0

515 stars 52 forks source link

disabling cross-instance storage test, node tcp pause/resume test #308

Closed agallant closed 8 years ago

agallant commented 8 years ago

@ryscheng @willscott

Not high priority, but I'm re-disabling this test - I had enabled it because it was passing in freedom(-for-firefox, -for-chrome), but I'm now revisiting freedom-for-node for the first time in awhile (https://github.com/freedomjs/freedom-for-node/tree/soycode-cleanup) and have found that this test does not pass in it.

That, combined with the good question raised by the TODO of whether this behavior is even desired, suggests I should disable the test. However if either of you think this test should be desired behavior (since it is what freedom does in most environments) then I can leave it enabled and focus on getting the node flavor to pass it.

coveralls commented 8 years ago

Coverage remained the same at 80.468% when pulling 34cde3b009866487a011cb0e5b2c4295d97b3582 on soycode-aligntests into 2ed50381e54b6c2d520e828c2f637ec2867c8251 on master.

agallant commented 8 years ago

Now also disabling the pause/resume TCP test for node, which actually nominally passed but cause strange issues where it still ran code after its done() call (randomly and spuriously breaking following tests). This one is definitely worth more properly revisiting, but for now it'd be helpful to have Shippable/Travis green for node again.

coveralls commented 8 years ago

Coverage remained the same at 80.468% when pulling f068e158b1808fb8aeb7d3ad64f6e90276e3b1be on soycode-aligntests into 2ed50381e54b6c2d520e828c2f637ec2867c8251 on master.

willscott commented 8 years ago

pause resume seems like something we expect to be called and that node should be able to handle gracefully. the cross-instance storage one I don't think we care about either way, so path of least resistance is good there.

agallant commented 8 years ago

Agreed pause/resume should work in node, and it seems to - the bug is that later tests fail, which may have more to do with the test than the implementation (or at least some node-specific quirk with how it runs the test). I'll see if I can debug it but if I can't fix it I'd still be tempted to disable it in node just so the Shippable/Travis alerts become more meaningful.

agallant commented 8 years ago

Came up with a mildly better approach - just ensure the socket is only closed once.

coveralls commented 8 years ago

Coverage remained the same at 80.468% when pulling 03cf9526203781fa65c6744390216be2249e9302 on soycode-aligntests into 2ed50381e54b6c2d520e828c2f637ec2867c8251 on master.