Refactoring tests around JS promise.
The problem:
We had a JS promise
, but we never set a catch
or finally
.
This wasn't a problem, until it became one.
I am not joint to go into details of why it is a problem not to catch
a reject
.
Our issue was that there was an error:
Uncaught (in promise) ....
After that the JS of the webpage was broken. Simple showing of a tooltip broke the webpage.
Our solution:
const promise = new Promise...
promise.catch(() =>{
do something to catch the erorr.
}).finally(() => {
do something after a success or error.
})
The tests:
I am going to try to make my point clear by showing show our test suite's name changed across the development.
First I had to refactor the tests, because we found out that if I add the fix and the tests another person has no way to understand what happened.
Start
Tests:
IS.StepsTree.LoaderSpec
onCoreInit
downloads file and notifies listeners for stepsTreeLoaded
the IS.StepsTree.LoadedEvent contains the true root
notifies for stepsLoadingProgress FINISHED
[Refactoring] Moving loader to this.
We defined loader more than once and I attached it to this
scope which is share in the tests.
[Refactoring] Moving DummyIProvider to be defined at the top of the document and renaming him to DummyResolveIProvider.
We had a class DummyIProvider
that was defined deep in the examples. I moved it to the top in order to be clear where it is defined.
Also I laid the ground work to link it to the resolve
case by renaming it to DummyResolveIProvider
.
Tests:
IS.StepsTree.LoaderSpec
onCoreInit
downloads file and notifies listeners for stepsTreeLoaded
the IS.StepsTree.LoadedEvent contains the true root
notifies for stepsLoadingProgress FINISHED
[Refactoring] Moving the DummyResolveIProvider tests that test for when it resolves to a separate describe.
Here I am ready to add the fix and the tests for the fix.
Tests:
IS.StepsTree.LoaderSpec
onCoreInit
onStepsTreeTryLoadFiles when getStepsTree resolves
downloads file and notifies listeners for stepsTreeLoaded
the IS.StepsTree.LoadedEvent contains the true root
notifies for stepsLoadingProgress FINISHED
82283fa [Loader] When the promise gets rejected we now have a catch, to catch the message. And the ... event is fired in a finally.
Here I add another dummy class: DummyRejectIProvider
which as you can guess rejects. Its role is in onStepsTreeTryLoadFiles when getStepsTree rejects
Tests:
IS.StepsTree.LoaderSpec
onCoreInit
onStepsTreeTryLoadFiles when getStepsTree resolves
downloads file and notifies listeners for stepsTreeLoaded
the IS.StepsTree.LoadedEvent contains the true root
notifies for stepsLoadingProgress FINISHED
onStepsTreeTryLoadFiles when getStepsTree rejects
the messages is catched
notifies for stepsLoadingProgress FINISHED
Here I had the feature and tests in but I was not happy, because the feature was indeed not about onCoreInit
, but about onStepsTreeTryLoadFiles
. So the refactoring begins now:
[Refactoring] Adding the same call in beforeEach
[Refactoring] Changed the setup from onCoreInit to onStepsTreeTryLoadFiles.
These two come almost together. I had to extract all the logic and fix the setup. Because as you can see in the previous commit the setup came from onCoreInit
. But now our setup needed to come only from onStepsTreeTryLoadFiles
, which was kind of tricky, because I had to manage a lot of promises. Because both onStepsTreeTryLoadFiles
and onCoreInit
indeed return a promise.
Tests:
IS.StepsTree.LoaderSpec
onStepsTreeTryLoadFiles
getStepsTree resolves
downloads file and notifies listeners for stepsTreeLoaded
the IS.StepsTree.LoadedEvent contains the true root
notifies for stepsLoadingProgress FINISHED
getStepsTree rejects
the messages is catched
notifies for stepsLoadingProgress FINISHED
[Refactoring] Moved this.extensionDefs and this.stepsTreeLoadedListener to a before each in the top describe.
[Refactoring] Moving a duplicated call to a upper describe beforeEach.
Here I had to lay another strong foundation, because I needed to add tests for onCoreInit, because after changing the setup we basically stopped testing that it is doing it's job.
Added test for onCoreInit.
Tests:
IS.StepsTree.LoaderSpec
onStepsTreeTryLoadFiles
getStepsTree resolves
downloads file and notifies listeners for stepsTreeLoaded
the IS.StepsTree.LoadedEvent contains the true root
notifies for stepsLoadingProgress FINISHED
getStepsTree rejects
the messages is catched
notifies for stepsLoadingProgress FINISHED
onCoreInit
schedules a TryLoadFilesEvent
v2.0.4
Last but not leas releasing the new version with the fix for the catch of the promise.
It took me around 4 hours, to refactor everything and make the proper setup. \
You may think that that is madness, because I took 4 hours on a 5 line fix. But on the contrary it was way worth it, because now the test are decoupled, properly set and everything is in place.