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

Refactor code to remove test redundancy in limit_price_computer #1054

Open
sonaalKant opened this issue Jun 19, 2024 · 9 comments
Open

Refactor code to remove test redundancy in limit_price_computer #1054

sonaalKant opened this issue Jun 19, 2024 · 9 comments
Assignees

Comments

@sonaalKant
Copy link
Collaborator

We want to remove the redundancy caused due to testing classes in the same hierarchy. We will refactor the common tests in the Testcase refer https://github.com/cryptokaizen/cmamp/blob/7a960f6d8f22d73dbb98a2c65552ce508e872074/docs/coding/all.write_unit_tests.how_to_guide.md?plain=1#L342

We would like to do the same for oms/limit_price_computer/test/

@mayank922
Copy link
Contributor

Hi @samarth9008

I am trying to implement the following things in the test file
https://github.com/kaizen-ai/kaizenflow/blob/master/oms/limit_price_computer/test/test_limit_price_computer_using_spread.py

  1. Create a Helper class which can contain these 3 helper function

    def _calculate_price_helper
    A helper function for test_compare_latest_and_average_price
    A helper function of get_object

  2. Having a single class and adding the functions of class Test_LimitPriceComputerUsingSpread_Obj_to_str1 to it

I wanted to know that I am in the right direction or do you suggest something else?

@samarth9008
Copy link
Collaborator

The right guy to answer this is @sonaalKant

@mayank922
Copy link
Contributor

Hi @sonaalKant

For the second test file : test_limit_price_computer_using_volatility

In the Test 3

  1. Do we need to verify the results of each value in the list ?
    reason being the volatility_multiple is already being passed in other test cases and we can just check for multiplier as a List.

  2. Having a single class and adding the functions of Test_LimitPriceComputerUsingVolatility_Obj_to_str1 to it

@mayank922
Copy link
Contributor

Hi @sonaalKant

I have implemented the changes to both the test files. Would you like to suggest something else?

@sonaalKant
Copy link
Collaborator Author

Always link your PR with the issue, so that it is easier for the reviewer to navigate.

@samarth9008
Copy link
Collaborator

Always link your PR with the issue, so that it is easier for the reviewer to navigate.

Collaborators on kaizen do not have access to zenhub

@sonaalKant
Copy link
Collaborator Author

Collaborators on kaizen do not have access to zenhub

I meant they can still link it by mentioning the issue number in comments. #<issue_number>

@mayank922
Copy link
Contributor

Hi @sonaalKant

I apologize for creating confusion here. I haven't created a PR yet, I have just made the changes in my branch.
I was asking if I was in the right direction.

I will create a PR today with the changes implemented so that anyone can review it.

@sonaalKant
Copy link
Collaborator Author

It is easier to comment and leave suggestion on a PR.

I will create a PR today with the changes implemented so that anyone can review it.

yeah, that will be helpful. Thanks.

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

No branches or pull requests

3 participants