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: remove CreditNft mock #762

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

rndquu
Copy link
Member

@rndquu rndquu commented Aug 14, 2023

This PR:

  1. Removes the CreditNft mock contract
  2. Refactors tests to use the real CreditNft contract

@ubiquibot
Copy link

ubiquibot bot commented Aug 14, 2023

@molecula451
Copy link
Member

are we purging many of the mocks contracts if not all? @rndquu

@rndquu
Copy link
Member Author

rndquu commented Aug 15, 2023

are we purging many of the mocks contracts if not all? @rndquu

All of them

@0x4007
Copy link
Member

0x4007 commented Aug 16, 2023

are we purging many of the mocks contracts if not all? @rndquu

All of them

I thought the purpose of mock contracts was to make writing unit tests easier? Why should we remove them? Is it insufficient to continue using mocks for unit tests?

@rndquu
Copy link
Member Author

rndquu commented Aug 16, 2023

are we purging many of the mocks contracts if not all? @rndquu

All of them

I thought the purpose of mock contracts was to make writing unit tests easier? Why should we remove them? Is it insufficient to continue using mocks for unit tests?

You're right, mocks make unit tests easier. On the frontend, for example, you can mock API calls. On the backend, for example, you can mock DB queries. But in the contracts it is not a big deal to deploy everything you need locally.

Check the mocks folder. It arises a question:

  • Why do we need to mock CreditNft, CreditToken, DollarToken, UbiquiStick when we can simply use the real contracts?

I understand why Curve's contracts were mocked (because they should be compiled with vyper and it is tricky to set it up in foundry) but other contract should not have been mocked. Your mock implementation can differ from a real contract hence there might be errors in unit tests while using the real contracts make sure that everything will work fine in production.

@molecula451
Copy link
Member

I agree in testing real contracts on forked chain than having mocks and mocks for each update

@0x4007 0x4007 merged commit 66e97ec into ubiquity:development Aug 16, 2023
10 checks passed
@rndquu rndquu deleted the refactor/rmv-credit-nft-mock branch August 17, 2023 11:41
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.

3 participants