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

Ithaca increase test coverage and fix tzstats #610

Merged
merged 20 commits into from Dec 15, 2022
Merged

Conversation

vkresch
Copy link
Contributor

@vkresch vkresch commented Aug 19, 2022

Changelog:

  • Test reactivation
  • API consistency test so that tzstats = rpc = tzkt
  • Reward API cached data update
  • Fix tzstats reward api
  • Fix rpc reward api

Resolves #600

To fix tzstats I am currently assuming since tzkt api is working it is the standard to which we should compare values for now.

Notes:
I readded some tests with actual API calls. I think the best way to currently test TRD is a mix between real and mock API calls.
With real API calls we can detect faster deprecated API endpoints IMO thus we can adjust/fix things quicker. The real API calls should only do non changing data calls. That means we call only data which is static and can never change e.g. the balance in the past cycles. I know it slows down the merge process and that there is a discussion about mocking all the API calls but that way we can make sure TRD works.

Mock API calls we should use if we want to test some recent data which will change in the future. E.g. the current balance of a delegator or the data is to big to query each time (performance issue).

IMPORTANT:
Please use in future PRs:
@pytest.mark.skip(reason="no way of currently testing this")
if you really need to disable a test instead of removing or commenting it out.
Also provide a reason for that for future developers who do not know why the test was disabled.

Issues detected during test activation (tick means this PR solved this issue):

  • Querying the baking rights via pRPC causes currently a 429 To many requests error. I created an issue at the tezos repo (https://gitlab.com/tezos/tezos/-/issues/3791). Let me know if this is the right place to post the issue.
  • RPC reward API did not filter out zero staking balance delegates
  • Updated potential_endorsement_rewards of tzstats by divinding with 1e6 to approximatly match tzkt futureRewards

Work effort: 32h

@vkresch vkresch marked this pull request as draft August 19, 2022 16:32
@nicolasochem
Copy link
Contributor

Thanks, the suggestion to use a mix of mock and real rpc calls makes sense. Also, I'll make sure to skip the pytest tests instead of commenting them out.

src/Constants.py Outdated Show resolved Hide resolved
@vkresch
Copy link
Contributor Author

vkresch commented Sep 12, 2022

Need to implement some final tests for the reward api of tzstats and then the PR can be reviewed and merged.

@vkresch
Copy link
Contributor Author

vkresch commented Sep 22, 2022

@nicolasochem @jdsika can you both test the the tzstats reward api? I would like to merge this PR as soon as possible because it's getting to big. Thanks!

I still did not removed the release_override and currently there are issues with rpc which do not match with tzkt but that will be for another PR.

@vkresch vkresch requested a review from jdsika September 22, 2022 01:06
src/Constants.py Outdated Show resolved Hide resolved
@jdsika jdsika added the critical Critical label Sep 22, 2022
@jdsika jdsika added this to the v11.0 (Ithaca) milestone Sep 22, 2022
@jdsika
Copy link
Contributor

jdsika commented Nov 18, 2022

@vkresch I think you should have a look at your tests and then we can test it, right? I will be more active now and push a release

@jdsika
Copy link
Contributor

jdsika commented Dec 7, 2022

@vkresch please make sure this gets ready this week!

@vkresch vkresch requested a review from jdsika December 11, 2022 01:27
@vkresch
Copy link
Contributor Author

vkresch commented Dec 11, 2022

@jdsika @nicolasochem ready for review/merge.

@rvermootenct
Copy link
Contributor

I have spent time looking through the code and the tests, as well as running and confirming that my reports are the same while on ghostnet and using dry run while on mainnet on the different reward network providers. From what I can see this PR is ready to go. Awesome work @vkresch

I will add a disclaimer that I am still new to TRD and may have missed important questions.

Copy link
Contributor

@rvermootenct rvermootenct left a comment

Choose a reason for hiding this comment

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

I have spent time looking through the code and the tests, as well as running and confirming that my reports are the same while on ghostnet and using dry run while on mainnet on the different reward network providers. From what I can see this PR is ready to go. Awesome work @vkresch

I will add a disclaimer that I am still new to TRD and may have missed important questions.

@jdsika jdsika merged commit a6812fd into master Dec 15, 2022
@jdsika jdsika deleted the extension/ithaca_polish branch December 15, 2022 15:27
@nicolasochem
Copy link
Contributor

thank you @vkresch @rvermootenct

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical Critical
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reminder: Ithaca2 branch
4 participants