kredits/contracts
9
1
Fork 0
Code Issues 21 Pull Requests Actions Projects 1 Releases 8 Activity

[WIP] Solidity contracts tests #106

Closed
haythem96 wants to merge 17 commits from tests/contracts into master
pull from: tests/contracts
merge into: :master
Conversation 18 Commits 17 Files Changed 27
+97054 -71513
haythem96 commented 2019-04-17 22:39:36 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Unit tests for Kredits smart contracts

Unit tests for Kredits smart contracts
fsmanuel commented 2019-04-17 22:55:58 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@haythem96 pretty cool to see how to test the contracts. I'm wondering why the pull upstream master commit reverts the pagination changes? I guess you need to rebase/merge master again...

@haythem96 pretty cool to see how to test the contracts. I'm wondering why the `pull upstream master` commit reverts the pagination changes? I guess you need to rebase/merge master again...
haythem96 commented 2019-04-19 02:36:32 +00:00 (Migrated from github.com)
Copy Link
Copy Source

for what this unused variable ?

for what [this](https://github.com/haythem96/kredits-contracts/blob/5368a3d7d819bf498ca7b0f1d9a690ffacf1a48c/apps/contributor/contracts/Contributor.sol#L34) unused variable ?
bumi commented 2019-04-19 08:49:33 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@haythem96 you mean root? I missed that in this commit: https://github.com/haythem96/kredits-contracts/commit/766463b57b87b2233a5bd681310da7332b9db904#diff-b24176ed2ec209da948c7ef4f480abe1L36
but the kit is still calling the function like that.

@haythem96 you mean `root`? I missed that in this commit: https://github.com/haythem96/kredits-contracts/commit/766463b57b87b2233a5bd681310da7332b9db904#diff-b24176ed2ec209da948c7ef4f480abe1L36 but the kit is still calling the function like that.
raucao (Migrated from github.com) reviewed 2019-04-19 08:58:19 +00:00
apps/contributor/test/contributor.js
raucao (Migrated from github.com) commented 2019-04-19 08:58:19 +00:00
Copy Link
Copy Source

This doesn't actually check for an updated hash afaics.

This doesn't actually check for an updated hash afaics.
haythem96 (Migrated from github.com) reviewed 2019-04-20 14:32:33 +00:00
apps/contributor/test/contributor.js
haythem96 (Migrated from github.com) commented 2019-04-20 14:32:32 +00:00
Copy Link
Copy Source

yup...solved in this commit

yup...solved in this [commit](https://github.com/67P/kredits-contracts/pull/106/commits/ab6d6a40251a9589d99886b0e53bb86b74f50080)
haythem96 (Migrated from github.com) reviewed 2019-05-04 22:00:26 +00:00
apps/contribution/test/contribution.js
@@ -0,0 +96,4 @@
{ from: root }
);
//acl.grantPermission(contribution, token, await token.MINT_TOKEN_ROLE(), {from: root});
haythem96 (Migrated from github.com) commented 2019-05-04 22:00:26 +00:00
Copy Link
Copy Source

I have a problem here granting the mint token permission to the contribution app...

I have a problem here granting the mint token permission to the contribution app...
haythem96 (Migrated from github.com) reviewed 2019-05-04 22:01:48 +00:00
apps/proposal/test/proposal.js
@@ -0,0 +2,4 @@
// eslint-disable-next-line no-undef
const Proposal = artifacts.require("Proposal.sol");
const { Contributor, getContributorContract } = require("../../contributor/artifacts");
haythem96 (Migrated from github.com) commented 2019-05-04 22:01:48 +00:00
Copy Link
Copy Source

idk why I have an error here importing other apps contracts?

idk why I have an error here importing other apps contracts?
bumi (Migrated from github.com) reviewed 2019-05-05 08:25:28 +00:00
apps/contribution/test/contribution.js
@@ -0,0 +96,4 @@
{ from: root }
);
//acl.grantPermission(contribution, token, await token.MINT_TOKEN_ROLE(), {from: root});
bumi (Migrated from github.com) commented 2019-05-05 08:25:27 +00:00
Copy Link
Copy Source

what do you mean? what's the error?

what do you mean? what's the error?
bumi (Migrated from github.com) reviewed 2019-05-05 08:29:41 +00:00
apps/proposal/test/proposal.js
@@ -0,0 +2,4 @@
// eslint-disable-next-line no-undef
const Proposal = artifacts.require("Proposal.sol");
const { Contributor, getContributorContract } = require("../../contributor/artifacts");
bumi (Migrated from github.com) commented 2019-05-05 08:29:41 +00:00
Copy Link
Copy Source

looks to me like artifacts might not defined in the contributor/artifacts.js file.
artifacts.require('Contributor') does not work here? because the proposal app does not know about the Contributor.sol?

from where is truffle loading the contracts?

looks to me like `artifacts` might not defined in the contributor/artifacts.js file. `artifacts.require('Contributor')` does not work here? because the proposal app does not know about the Contributor.sol? from where is truffle loading the contracts?
haythem96 (Migrated from github.com) reviewed 2019-05-05 10:31:06 +00:00
apps/contribution/test/contribution.js
@@ -0,0 +96,4 @@
{ from: root }
);
//acl.grantPermission(contribution, token, await token.MINT_TOKEN_ROLE(), {from: root});
haythem96 (Migrated from github.com) commented 2019-05-05 10:31:06 +00:00
Copy Link
Copy Source

a revert...

a revert...
haythem96 (Migrated from github.com) reviewed 2019-05-05 15:26:41 +00:00
apps/proposal/test/proposal.js
@@ -0,0 +2,4 @@
// eslint-disable-next-line no-undef
const Proposal = artifacts.require("Proposal.sol");
const { Contributor, getContributorContract } = require("../../contributor/artifacts");
haythem96 (Migrated from github.com) commented 2019-05-05 15:26:40 +00:00
Copy Link
Copy Source

I did the same with the Contribution app to access to the Token app contracts, and it worked...
If I understand correctly artifacts.require('Contributor.sol') in Contributor app should b able to access that contract inside that app contracts folder... or no ?

I did the same with the Contribution app to access to the Token app contracts, and it worked... If I understand correctly `artifacts.require('Contributor.sol')` in Contributor app should b able to access that contract inside that app contracts folder... or no ?
fsmanuel commented 2019-05-11 13:02:03 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@haythem96 You should run npm run lint:wrapper --fix and npm run lint:contract-tests --fix to get the fixable lint errors fixed. After that see what lint errors are left and fix them by hand.

@haythem96 You should run `npm run lint:wrapper --fix` and `npm run lint:contract-tests --fix` to get the fixable lint errors fixed. After that see what lint errors are left and fix them by hand.
👍 1
haythem96 commented 2019-05-12 13:36:17 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@haythem96 You should run npm run lint:wrapper --fix and npm run lint:contract-tests --fix to get the fixable lint errors fixed. After that see what lint errors are left and fix them by hand.

I did that...had some issue with the lint rules(e.g in the contracts test it show 'artifacts' is not defined or 'before' is not defined... so I just disabled some rules in the tests files

> @haythem96 You should run `npm run lint:wrapper --fix` and `npm run lint:contract-tests --fix` to get the fixable lint errors fixed. After that see what lint errors are left and fix them by hand. I did that...had some issue with the lint rules(e.g in the contracts test it show `'artifacts' is not defined` or `'before' is not defined`... so I just disabled some rules in the tests files
👍 1
haythem96 commented 2019-06-16 19:23:01 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Contributor app tests works... still have issue with Contribution app, the getTokenContract() always return address(0) in the tests

Contributor app tests works... still have issue with Contribution app, the `getTokenContract()` always return `address(0)` in the tests
raucao commented 2019-06-17 07:16:07 +00:00 (Migrated from github.com)
Copy Link
Copy Source

This is turning into quite the monolithic monster pull request imo.

@haythem96 You think we can split this up, so it's easier to review, merge, and collaborate?

For example I could imagine to first just merge the basic test suite setup, so that the test command simply works and it's possible to add tests. Then we could merge whatever tests you already have (I guess Contributor), and then steadily add more to cover all code.

What do you think?

This is turning into quite the monolithic monster pull request imo. @haythem96 You think we can split this up, so it's easier to review, merge, and collaborate? For example I could imagine to first just merge the basic test suite setup, so that the test command simply works and it's possible to add tests. Then we could merge whatever tests you already have (I guess Contributor), and then steadily add more to cover all code. What do you think?
👍 1
raucao commented 2019-06-17 07:19:49 +00:00 (Migrated from github.com)
Copy Link
Copy Source

P.S.: The way kredits are earned with the labels, it should actually encourage the smallest possible pull requests, because the highest kredits amount is "large" (kredits-3), which shouldn't be more than a few days of work (equivalent to 10 small contributions, or a bit more than 3 medium ones). And in this case, I'm certain it should be more than a single large one.

P.S.: The way kredits are earned with the labels, it should actually encourage the smallest possible pull requests, because the highest kredits amount is "large" (`kredits-3`), which shouldn't be more than a few days of work (equivalent to 10 small contributions, or a bit more than 3 medium ones). And in this case, I'm certain it should be more than a single large one.
👍 1
haythem96 commented 2019-06-17 09:39:11 +00:00 (Migrated from github.com)
Copy Link
Copy Source

okay will close this one for now & do others small PRs

okay will close this one for now & do others small PRs
raucao commented 2019-06-17 09:45:09 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Cool!

Cool!

Pull request closed

This pull request cannot be reopened because the branch was deleted.
This pull request is marked as a work in progress.
Sign in to join this conversation.
Reviewers
No Reviewers
raucao
bumi
haythem96
Labels
Clear labels
good first issue
Good for newcomers
 ipfs
rsk
scaling
bug
Something is not working
 dev environment
Config, builds, CI, deployment, etc.
 docs
Documentation
 duplicate
This issue or pull request already exists
 enhancement
Improving existing functionality
 feature
New functionality
 idea
Something to consider
 invalid
Not a bug
 kredits-1
Small contribution
 kredits-2
Medium contribution
 kredits-3
Large contribution
 question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
 ui/ux
User interface, process design, etc.
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/contracts#106