Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

✅ Adding tests for trading executor #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thibaultyou
Copy link
Owner

No description provided.

@thibaultyou
Copy link
Owner Author

thibaultyou commented Sep 29, 2021

@mrsegen can I have some feedbacks on this one ?

I have some issues with open handles with Jest since I've played with setInterval function and timers, maybe you have a workaround for it ? (try running npm run test --detectOpenHandles)

@leifjones
Copy link
Collaborator

So far I had some success with at least getting it to pass with jest.useFakeTimers(); atop the file. Not sure yet if it's testing/verifying what's needed with that change. (If I'm remembering correctly... I'm away from the computer now and typing from memory.)

If going with it, check the jest documentation for it. I think I put a jest.useRealTimers(); within an afterEach(). That should be enough to get it passing.

Possible next steps, for subsequent iterations, if it makes sense for what's being tested:

  • Maybe see if it makes sense to do the expect(setTimeout).toHaveBeenCalledWith(this.processTrades, durationOfDelay) - similar to the example on the documentation site.

  • I was having trouble with it wanting setTimeout to be a mock. I wonder if 'use strict' is required for that to work?

  • I separated out the first parameter to a standalone processTrades (plural) function so that the toHaveBeenCalledWith might work.

  • It might be worth testing the durations - since the need to vary by exchange. I wonder if ccxt would eventually try to support that somehow, but if we can fix it here, no reason not to.

@thibaultyou thibaultyou added the enhancement New feature or request label Oct 1, 2021
@thibaultyou
Copy link
Owner Author

@mrsegen thank you for the advices, your contributions are always highly appreciated 🙏

I'll try using statements in afterEach blocks this may fix the warnings.
Testing durations may also be better using toHaveBeenCalledWith that using steps in timers, I'll try this out.

@leifjones
Copy link
Collaborator

So far, when I went deeper with that, and even making tests of the exchange services, I've been running into a strange errors during testing: Something about node thinking SpotExchangeService is undefined in lines such as:
class BinanceExchangeService extends SpotExchangeService

Let me know if you run into that, and/or find a way around it. So far I've explored trying to address apparent circular dependencies. I haven't made much progress yet.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants