Warn when unlisten called multiple times

[aij]

Per discussion on #436 , emit a warning while preserving existing behavior.

[mjackson]

Is one of our tests calling unlisten more than once? Can you tell which one it is and fix it?

[mjackson]

A warning without a fix for the bad behavior is just going to annoy people.

[aij]

That was my thought as well, which was why I included a fix to the tests in the first PR... Did you want the same fix or or would you prefer something different?

The problem is in execSteps.js, execNextStep() calls cleanup() twice for the last step, first inside the try and then again inside the catch.

[aij]

And I guess the followup question is: Once the tests no longer leave the history object in an inconsistent state, should they keep using the same one instead of throwing it out and creating a new one as they currently do? (I think there is still some merit to creating a new one each time, in that it avoids state from earlier tests from potentially breaking later tests.)

[mjackson]

The problem is in execSteps.js, execNextStep() calls cleanup() twice for the last step, first inside the try and then again inside the catch.

Good catch, thank you.

I didn't like the "fix" in the first PR because it was just setting unlisten to a no-op instead of fixing the fact that it was being called twice.

should they keep using the same one instead of throwing it out and creating a new one as they currently do?

No, I like creating a new object each time. It helps to know we're starting with fresh state in every test.

[aij]

Could we still include the warning though? I'm more worried about this happening in real code rather than just in the tests.

Also, sorry I may have misunderstood what you didn't like earlier. I thought you were opposed to making the unlisten function idempotent in case anyone might be relying on the old behavior. I hadn't realized updating the function pointer in the cleanup function was (also?) what you found objectionable.

[mjackson]

Instead of including a warning, how about we just make sure we don't decrement the listenerCount if it's called multiple times? Ideally, it wouldn't matter if they did call unlisten many times. We just don't want it to affect our internal state.