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

Tests for CollateralToken and ERC20 #17

Conversation

kelvintyb
Copy link
Contributor

@kelvintyb kelvintyb commented Jun 17, 2018

Description

Add specs for CollateralToken and ERC20

Checklist
  • Linter status: 100% pass
  • Changes don't break existing behavior
  • Tests coverage hasn't decreased
Refers/Fixes

Fixes: #3
Fixes: #4

@eswarasai eswarasai self-requested a review June 17, 2018 16:39
@eswarasai eswarasai changed the title add variables specs for CollateralToken and ERC20 [WIP] Tests for CollateralToken and ERC20 Jun 17, 2018
@eswarasai
Copy link
Contributor

@kelvintyb -- Any update on this? Let me know if you need any help in case you're stuck. Also looks like there are some conflicts. Please do rebase your branch with that of upstream:develop and try to resolve the conflicts and commit the changes. Thanks!

@kelvintyb
Copy link
Contributor Author

Thanks @eswarasai, will reach out if I get stuck, this week is looking busy at work, but I will be able to work on this more over the weekend. Sorry about the delay!

@eswarasai
Copy link
Contributor

@kelvintyb -- Sure. No problem. Let us know once it's ready for review. Thanks :)

@kelvintyb
Copy link
Contributor Author

WIP update: Added more tests for methods on CollateralToken, and adjusted variables tests.

@eswarasai
Copy link
Contributor

@kelvintyb -- Thanks for the update. Let us know when the PR is ready for the review. Also please do rebase your branch with upstream:develop before passing onto review.

@kelvintyb kelvintyb force-pushed the feature/collateral_token_erc20_tests branch from 072131a to b5bf7fc Compare June 25, 2018 15:52
@kelvintyb kelvintyb changed the title [WIP] Tests for CollateralToken and ERC20 Tests for CollateralToken and ERC20 Jun 25, 2018
@kelvintyb
Copy link
Contributor Author

@eswarasai rebased my branch on the latest upstream:develop; PR is ready for review.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 48

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 92.086%

Totals Coverage Status
Change from base Build 47: 0.9%
Covered Lines: 236
Relevant Lines: 258

💛 - Coveralls

@eswarasai
Copy link
Contributor

@kelvintyb -- Can you please remove the methods test cases being duplicated in variables test cases for CollateralToken.test.ts? Example -- has approve, has transfer methods. Thanks!

@kelvintyb
Copy link
Contributor Author

Yep, done :)

Copy link
Contributor

@eswarasai eswarasai left a comment

Choose a reason for hiding this comment

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

LGTM!

@eswarasai eswarasai merged commit 38d11d7 into MARKETProtocol:develop Jun 27, 2018
@eswarasai
Copy link
Contributor

@kelvintyb -- I've just merged the PR. Just a heads up on the other issue you'd be tackling. So the methods approveTx, balanceOf should be under methods instead of variables test cases.

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