Skip to content
This repository was archived by the owner on Feb 15, 2022. It is now read-only.

Refactor backfill for unit testing #1214

Merged
merged 12 commits into from
Feb 2, 2018

Conversation

haxwell
Copy link
Contributor

@haxwell haxwell commented Jan 24, 2018

No functionality change, other than UI, and that is minor. The code has been rewritten with an eye toward DRY, SOLID, code. Take note of the

  • Consume-And-Process Service, which provides a structure for batch processing.
  • Resume-Marker Service, manages resume markers in memory, and flushes the new state with each batch.
  • CollectionsService, which uses direct Mongo DB objects, rather than Carlos' SOSA package. I just didn't see the value in SOSA. Can someone enlighten me?
  • The Stub Exchange, can we model all exchange objects this way? Its effectively the same as before, just cleaner.
  • Unit Tests and Mock Service Factories, how cool are mocks?

Please test test test!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
… Mongo collection from the database is returned. -- I investigated the SOSA Mongo module that db.trades and db.resume_markers wrapped. I couldn't determine a real benefit to SOSA, as opposed to going directly to the Mongo DB object. SOSA seems to be one abstraction too many. If I am wrong, it is easy enough to add it back. For now, simple wins.
…t-one test/_specs/commands/backfill/backfill.process.function.test.js' for example. To run all tests 'npm test'.
@haxwell
Copy link
Contributor Author

haxwell commented Jan 24, 2018

GDAX is the only exchange that seems to offer a reliable backfill source. In my testing, the other exchanges, fail in some way or another.

If you have access to other exchanges, please test this PR, and let me know if they are working as expected for you. Thanks!

Copy link
Owner

@DeviaVir DeviaVir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial readthrough LGTM, I'll run some tests as well

👍 on the tests 🥇


if (data !== undefined && typeof process.stdout.clearLine == 'function') {
process.stdout.clearLine();
process.stdout.write("Processed trades up to " + Moment(data.time).fromNow() + "." );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think we can show here exactly how many trades we have processed? I think that might give a better sense of what the bot is exactly doing (and what is taking so long)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.. I will make that change soon. I was going to have it display the time that the last trades were received/processed, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5261f02 !

@haxwell
Copy link
Contributor Author

haxwell commented Feb 1, 2018

My earlier comment is collapsed above, but just so its out there, I added the bit about showing the number of trades processed during backfilling in 5261f02

@haxwell haxwell mentioned this pull request Feb 1, 2018
@DeviaVir
Copy link
Owner

DeviaVir commented Feb 2, 2018

My tests look good 👍 merging

@DeviaVir DeviaVir merged commit 0af042f into DeviaVir:unstable Feb 2, 2018
@Haehnchen
Copy link
Contributor

fyi this merge breaks backfill in some way and so also live trading warmup:

new:
{ product_id: 'EOS-BTC' }
{ product_id: 'EOS-BTC', from: '4558738' }

old:
binance.EOS-BTC saved 1814 trades 1 days left
{ product_id: 'EOS-BTC', from: 1517569760767 }
.{ product_id: 'EOS-BTC', from: 1517572758190 }

there is something wring in starttime and endtime calcualtion. noticed via binance exchange, but possible an overall problem?

@dakreepy1
Copy link

I am also seeing this one breaking backfill as well. running backfill on gdax takes a very long time. When i use the trade option and it backfills the recent trade it pretty much sits saying backfilling....
i've opened an issue with my results a few minutes ago.

@Haehnchen
Copy link
Contributor

because it starts backfiling somewhere around 1970 :)

@haxwell
Copy link
Contributor Author

haxwell commented Feb 2, 2018

Noted... I will look into the issues tonight.

@station384
Copy link
Contributor

see #1271

@haxwell
Copy link
Contributor Author

haxwell commented Feb 3, 2018

See #1279.

@xdien xdien mentioned this pull request Feb 5, 2018
Heerpa pushed a commit to Heerpa/zenbot that referenced this pull request Feb 25, 2018
* If ObjectifySelector is passed an object, it will return it, unmodified.

* Removed calls to db.trades and db.resume_markers. Instead, the direct Mongo collection from the database is returned. -- I investigated the SOSA Mongo module that db.trades and db.resume_markers wrapped. I couldn't determine a real benefit to SOSA, as opposed to going directly to the Mongo DB object. SOSA seems to be one abstraction too many. If I am wrong, it is easy enough to add it back. For now, simple wins.

* Added scripts for testing. To call a single test, 'npm run-script test-one test/_specs/commands/backfill/backfill.process.function.test.js' for example. To run all tests 'npm test'.

* Refactored backfill for unit testability.

* Added check to see if exchange offers backfill data to backfill.js

* Only flush resumeMarkers if there is at least one existing.

* Minor, check for debugging before printing status

* ResumeMarkerService calls the callback when there are no ranges, and flush() is called.

* Removed debugger statement

* Now displaying a counter of the number of trades backfilled in the current session.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants