Tests for code in src/util #124
Conversation
Somewhat hacky attempt to also test that it does not resolve before the timer finishes.
| expect(rejectOpts.event.listeners.length).toBe(0) | ||
| }) | ||
|
|
||
| describe('should resolve with given value when a resolve-event occurs', () => { |
There was a problem hiding this comment.
Just a design suggestion. It would be more consistent to use it instead of using describe. Since in the original testing plan describe is used to denote collections of test belonging to modules.
So we could have a structure like
->describe
--->test
----->it
Or we could interchange it and test. Whichever is more suitable. I notice now that it is not present in the official documentation, but the implementation works for sure.
There was a problem hiding this comment.
Your design sounds prettier indeed. I just followed the jest documentation, which mentions nesting describe blocks but not nesting tests. I now see that it is documented too, it is shortly mentioned as being an alias for test, but that means it might not be nestable. Even if nesting tests would technically seem to work, I'd rather follow the documented approaches to avoid possible quirks later on (jest might change its implementation).
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 Some workarounds are discussed in this QA, e.g. using Node's My reasoning was that it should also be doable by just adding an 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 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. |
|
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 |
The two previous tests seemed to test the same thing, so removing one. Importing pouchdb seems unnecessary, just mocking put and get suffices. Adding tests for the case of update conflicts.
Not too happy introducing complexity in the mock.. I added a test for the mock itself.
Use a simple graph to test if it gives the correct results.
|
I think it's good enough. Time to merge and move on! |
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:
(I found some arguably hacky way to test
delayandevent-to-promisemore extensively, by using bogusawaitstatements to check if any promises resolved.)