Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit test for broker with multiple submit twap orders #1104

Merged
merged 26 commits into from
Aug 12, 2024

Conversation

jayati1397
Copy link
Contributor

@jayati1397 jayati1397 commented Jul 30, 2024

This PR is for the Issue #1103

This is the first draft to see if the solution is correct.
We are testing broker with two twap orders being submitted by just one instance of broker.
I didn't use test_submit_twap_orders as it creates one instance of broker inside the function and calls submit_twap_orders once

Ran pytest and it passes.
Ran the linter too.
@samarth9008 please check and let me if the solution seems correct

@jayati1397 jayati1397 requested a review from samarth9008 July 30, 2024 04:38
@samarth9008
Copy link
Collaborator

Lets follow the process of naming the PR correctly as we did in the previous in the PRs.

Copy link
Collaborator

@samarth9008 samarth9008 left a comment

Choose a reason for hiding this comment

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

High level looks good. Lets improve minor things based on the reviews of the previous PRs.

One question from my side.

oms/broker/ccxt/test/test_ccxt_broker.py Show resolved Hide resolved
@jayati1397 jayati1397 changed the title update Unit test for broker with multiple submit twap orders Jul 30, 2024
@jayati1397 jayati1397 marked this pull request as ready for review July 30, 2024 19:36
Copy link
Collaborator

@Sameep2808 Sameep2808 left a comment

Choose a reason for hiding this comment

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

Few Changes

oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
@jayati1397 jayati1397 requested a review from Sameep2808 July 31, 2024 14:23
@jayati1397 jayati1397 force-pushed the KaizenTask1103_test_broker_with_multiple_orders branch from 6ffc72b to a042636 Compare July 31, 2024 17:49
@jayati1397 jayati1397 requested a review from Sameep2808 July 31, 2024 17:54
Copy link
Collaborator

@Sameep2808 Sameep2808 left a comment

Choose a reason for hiding this comment

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

The logic looks good but the code is to complicated to read right now. Lets use a for loop to call _submit_twap_orders and check results as I see we are repeating the same code twice

oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
@jayati1397 jayati1397 requested a review from Sameep2808 July 31, 2024 22:09
Copy link
Collaborator

@Sameep2808 Sameep2808 left a comment

Choose a reason for hiding this comment

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

Good Job. Few changes.

Just a note we don't leave blank lines in-between code so just remove where ever applicable

oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
@jayati1397 jayati1397 force-pushed the KaizenTask1103_test_broker_with_multiple_orders branch from 7965757 to 976acc1 Compare August 1, 2024 02:33
Copy link
Collaborator

@Sameep2808 Sameep2808 left a comment

Choose a reason for hiding this comment

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

Few Nits

oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
@jayati1397 jayati1397 force-pushed the KaizenTask1103_test_broker_with_multiple_orders branch from 25854d7 to 4854961 Compare August 1, 2024 04:41
jayati1397 added 2 commits August 1, 2024 00:49
@jayati1397 jayati1397 force-pushed the KaizenTask1103_test_broker_with_multiple_orders branch from cbbdde6 to 943655e Compare August 1, 2024 18:21
@jayati1397 jayati1397 requested a review from Sameep2808 August 1, 2024 18:46
Copy link
Collaborator

@Sameep2808 Sameep2808 left a comment

Choose a reason for hiding this comment

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

1 Nit. LGTM

oms/broker/ccxt/test/test_ccxt_broker.py Outdated Show resolved Hide resolved
jayati1397 added 2 commits August 2, 2024 18:49
@jayati1397 jayati1397 force-pushed the KaizenTask1103_test_broker_with_multiple_orders branch from 4f7d6bd to 84319ee Compare August 5, 2024 04:40
Copy link
Collaborator

@Sameep2808 Sameep2808 left a comment

Choose a reason for hiding this comment

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

1 Change. LGTM

…WithDynamicScheduler.test_submit_twap_orders_multiple_submission/output/actual_orders1.txt.tmp
@Sameep2808 Sameep2808 merged commit e62cc8e into master Aug 12, 2024
1 check passed
@Sameep2808 Sameep2808 deleted the KaizenTask1103_test_broker_with_multiple_orders branch August 12, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants