Refactoring tests around JS promise.

Bringing 3D models to life – example with Robotics mission model
August 12, 2019
Stimulus 1.1.1 to Stimulus 2.0.0 – possible regression
March 26, 2021

Refactoring tests around JS promise.

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.

Leave a Reply