kredits/contracts
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: kredits: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

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

@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

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

@haythem96 you mean root? I missed that in this commit: 766463b57b (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

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

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

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

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

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

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

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

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

@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

@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

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

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

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

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

Cool!

Cool!

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
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 Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
bkero bumi fsmanuel galfert greg haythem hueso raucao slvrbckt
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/contracts#106
No description provided.
Delete Branch "tests/contracts"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?