jupyterhub / binderhub

Run your code in the cloud, with technology so advanced, it feels like magic!
https://binderhub.readthedocs.io
BSD 3-Clause "New" or "Revised" License
2.54k stars 388 forks source link

JS: Remove dead state transition validation code #1771

Closed yuvipanda closed 11 months ago

yuvipanda commented 11 months ago

Soooo, because this.state was initialised to null and not 'start', this code was always actually dead. validateStateTransition had no branch for oldState being null, so always returned false. This meant this.state was never set to current state, and so in all callbacks, oldState was always null!

I validated this again by console.logging oldState and newState inside validateStateTransition. Here's the output for when a full build is performed:

null -> waiting
null -> fetching
null -> unknown
null -> building
null -> failed
null -> built
null -> launching

And for when launching a previously built image

null -> launching
null -> ready

I also put one just above where this.state is set, and that code was never called.

I was hoping to remove all this code anyway, as it doesn't actually validate nor test anything - that's really done by unit tests and integration tests. I thought this would hide bugs - but turns out, it was a bug itself, and hid nothing.

This also changes the signature of the callback for onChangeState, dropping oldState and newState. oldState was always null, and in our code, we actually don't use newState at all. The one place that was conditional on oldState being null is now unconditional, because oldState was always null.

Ref https://github.com/jupyterhub/binderhub/issues/774

yuvipanda commented 11 months ago

This is one way to get to 100% unit test coverage - just find code that was never used and get rid of it :)

consideRatio commented 11 months ago

Wieee thank you for working this @yuvipanda!