test(SFINT-3353): Test cleaning

[louis-bompart]

What

While investigating SFINT-3353, I discovered several issues in the UT
The actual 'fix' for SFINT-3353 will be done in a second term to have a clear split between refacto' and fix.

Improper use of expect:

https://jasmine.github.io/api/3.6/global.html#expect

TL;DR: Use matchers after expect. Always.

it('sometest'()=>{
   expect(false);
   expect(true);
}

This will pass. expect is not Assert. This will not pass:

it('sometest'()=>{
   expect(false).toBeTrue();
   expect(true).toBeTrue();
}

Issue with Stub, jasmine and yield function.

Various, various issues. My goal was to, whenever possible to 'flatten' and synchronize the tests, and avoid delay as much as possible (except when used to 'flush' the promise stack see waitForPromiseCompletions).

How to flatten delay

Doing an await(delay(()=>{}) ensure that all Promises are resolved before continuing. (assuming there's no setTimeout&co going on).
This is due to how microtasks and (macro)tasks work https://developer.mozilla.org/en-US/docs/Web/API/HTML_DOM_API/Microtask_guide

First, each time a task exits, the event loop checks to see if the task is returning control to other JavaScript code. If not, it runs all of the microtasks in the microtask queue. The microtask queue is, then, processed multiple times per iteration of the event loop, including after handling events and other callbacks.

So, long story short:

• All Promises before the await(delay(()=>{}) are ran (if they encounter a setTimeout so be it, they add it to the (macro)task queue, which will be executed once all promises are executed. If a promise is added, however, see the article, but from what I understand, it's executed before the next (macro)task
• SetTimeout, because it's at a 0 timeout is ready for cookin' on the pile, and resolve thus resolve the Promise that returns delay, freeing up the main process and, in our case, letting the other statement executes (often the assertions).

How did you test your tests.

By hand, most of them failed at first so, not a lot of choices (I won't say I checked all of them by hand, but a good 75%)

Further Improvements

• Apply proper unit-testing methodology by using Inversion of Control/Dependency Injections. This would allow proper 'black-boxing' and ensure that you can have more control over what's going on (e.g. with the ClickedDocumentList and the ExpendableList)
• Isolate the display logic, the state logic, and the query logic from each other. It would make the code more readable, and allow to test -thus maintain- more easily.

[mikegron]

nice catches!

[lbergeron]

Nice job!

[lbergeron]

Since delay() returns a Promise, shouldn't we use await delay(...) instead?

The way I understand it, the test currently exits before the expectations are verified.

[lbergeron]

...to each of it ... -> ...to each of its ...

[lbergeron]

Shouldn't it be expect(spyDispatchEvent.calledOnce).toBe(true) instead?

[lbergeron]

Shouldn't it be expect(spyDispatchEvent.calledOnce).toBe(true) instead?