WebMemex / webmemex-extension

📇 Your digital memory extension, as a browser extension
https://webmemex.org
Other
208 stars 45 forks source link

Tests for code in src/util #124

Closed Treora closed 6 years ago

Treora commented 6 years ago

Direct continuation of #116. @reficul31 I did not want to push my changes to your personal repo, if that's even possible, hence a new PR. Feel free (but not obliged) to review my changes, you probably know the art of testing better than I do after all your work.

Work in progress. So far I reviewed and revised:

Treora commented 6 years ago

Also I am not too sure about the await null implementation. Could you maybe expand a bit on the usage of that particular code?

I am not too sure about it either. The problem is that I would like to assert that a promise does not yet settle before the required timer/event triggers. Every approach based on awaiting a promise (e.g. jest's expect().resolves) is thus unusable here.

Some workarounds are discussed in this QA, e.g. using Node's util.inspect to read whether a promise is in 'pending' state, or using a Promise.race(p, Promise.resolve()) (quite clever, actually).

My reasoning was that it should also be doable by just adding an await statement, which will give control to the main event loop, which should then execute all pending promise handlers before returning to our test function. So after the await, if the callbacks had not been called, the promise must still be pending.

Though this seems to work in these tests, I am not sure if we have a guarantee that the callback would be run before our await statement continues (which itself is just syntactic sugar for setting up a callback). Are promise callbacks (including those from await) always executed in the same order that they were queued? I don't know. I am tempted to keep it for now and try to learn more about this (or find a better approach) sooner or later.

Another small point: even if this approach is correct, there is still the question whether it is reliable in other javascript engines; at least Firefox still has trouble with ordering promises/microtasks. Node/V8 is said to implement the specs correctly though, and is currently the only target for our tests.

reficul31 commented 6 years ago

Yeah, I totally agree with everything @Treora stated. I will have a more thorough look into it. Re reading the code I now full understand what we were trying to accomplish as well as why the need for await null was required. I think we should keep it as is now, and change later once a better implementation comes to light.

Treora commented 6 years ago

I think it's good enough. Time to merge and move on!