Contributor app tests #143

2019-06-26T20:34:24Z

haythem96 commented

2019-06-26 20:34:24 +00:00

(Migrated from github.com)

This PR add unit tests for the Contributor app contracts. #103

haythem96 commented

2019-07-05 21:11:37 +00:00

(Migrated from github.com)

can we merge this one ?

bumi (Migrated from github.com) requested changes 2019-07-06 15:29:27 +00:00

bumi (Migrated from github.com) left a comment

looks good to me. would be great if we can avoid the namehash dependency and use ethers.js for that.

that ganache-cli script is from aragon, isn't it?

looks good to me. would be great if we can avoid the namehash dependency and use ethers.js for that. that ganache-cli script is from aragon, isn't it?

apps/contributor/contracts/Contributor.sol

						
				@ -65,2 +65,4 @@

				  function updateContributorAccount(uint32 id, address oldAccount, address newAccount) public auth(MANAGE_CONTRIBUTORS_ROLE) {

				    require(newAccount != address(0), "invalid new account address");

				    require(getContributorAddressById(id) == oldAccount, "contributor does not exist");

bumi (Migrated from github.com) commented

2019-07-06 15:24:19 +00:00

👍 very good!

:+1: very good!

apps/contributor/test/contributor.js

bumi (Migrated from github.com) commented

2019-07-06 15:22:31 +00:00

we should be able to use the namehash from ethers:
const namehash = require('ethers').utils.namehash;
as ethers.js is already a dependency we do not need to add another dependency

we should be able to use the namehash from ethers: `const namehash = require('ethers').utils.namehash;` as ethers.js is already a dependency we do not need to add another dependency

raucao commented

2019-07-06 15:39:06 +00:00

(Migrated from github.com)

Are these test files linted? The formatting (e.g. indentation) seems to be all over the place from what I can see in the GitHub diff...

haythem96 (Migrated from github.com) reviewed 2019-07-06 16:07:41 +00:00

apps/contributor/test/contributor.js

haythem96 (Migrated from github.com) commented

2019-07-06 16:07:41 +00:00

cool will change that

haythem96 (Migrated from github.com) reviewed 2019-07-06 16:10:50 +00:00

apps/contributor/test/contributor.js

haythem96 (Migrated from github.com) commented

2019-07-06 16:10:49 +00:00

for the ganache script, it is basically the same aragon one, but I changed

run_tests() {
  echo "Running tests..."
  npx truffle test --network rpc "$@"
}

to

run_tests() {
  echo "Running tests..."
  npx aragon contracts test --network rpc "$@"
}

Because npx truffle test --network rpc "$@" didn't work for me for no reason...

for the ganache script, it is basically the same aragon one, but I changed ``` run_tests() { echo "Running tests..." npx truffle test --network rpc "$@" } ``` to ``` run_tests() { echo "Running tests..." npx aragon contracts test --network rpc "$@" } ``` Because `npx truffle test --network rpc "$@"` didn't work for me for no reason...

bumi commented

2019-07-06 16:17:49 +00:00

(Migrated from github.com)

ah ja, @haythem96 we used two spaces indentation on kredits. and you can run the linting using: npm run lint:contract-tests (though I had issues running the linter here right now. @skddc can you confirm this is correct?)

ah ja, @haythem96 we used two spaces indentation on kredits. and you can run the linting using: `npm run lint:contract-tests` (though I had issues running the linter here right now. @skddc can you confirm this is correct?)

raucao commented

2019-07-06 17:28:22 +00:00

(Migrated from github.com)

@skddc can you confirm this is correct?)

Confirm that what exactly is correct?

> @skddc can you confirm this is correct?) Confirm that what exactly is correct?

bumi commented

2019-07-07 09:36:07 +00:00

(Migrated from github.com)

@skddc that npm run lint:contract-tests is the right command to lint those files and that it works for you.
on my machine it does not give me any output.

@skddc that `npm run lint:contract-tests` is the right command to lint those files and that it works for you. on my machine it does not give me any output.

haythem96 commented

2019-07-07 09:37:19 +00:00

(Migrated from github.com)

@skddc that npm run lint:contract-tests is the right command to lint those files and that it works for you.
on my machine it does not give me any output.

I tried it also, no output...

> @skddc that `npm run lint:contract-tests` is the right command to lint those files and that it works for you. > on my machine it does not give me any output. I tried it also, no output...

fsmanuel commented

2019-07-07 22:11:23 +00:00

(Migrated from github.com)

@haythem96 we don't have an explicit indentation rule, yet.
You can add 'indent': ['error', 2] to the root .eslintrc.js in the rules section.

@haythem96 we don't have an explicit indentation rule, yet. You can add `'indent': ['error', 2]` to the root `.eslintrc.js` in the rules section.

👍 1

raucao commented

2019-07-08 08:13:25 +00:00

(Migrated from github.com)

that npm run lint:contract-tests is the right command to lint those files

I didn't create those commands and I never tried linting contract test files (no tests until very recently), so I don't know.

You can add 'indent': ['error', 2] to the root .eslintrc.js in the rules section.

Sounds like a good idea to me. Thanks, @fsmanuel.

> that `npm run lint:contract-tests` is the right command to lint those files I didn't create those commands and I never tried linting contract test files (no tests until very recently), so I don't know. > You can add `'indent': ['error', 2]` to the root `.eslintrc.js` in the rules section. Sounds like a good idea to me. Thanks, @fsmanuel.

haythem96 commented

2019-07-08 10:16:38 +00:00

(Migrated from github.com)

@haythem96 we don't have an explicit indentation rule, yet.
You can add 'indent': ['error', 2] to the root .eslintrc.js in the rules section.

Cool, fixed now.

> @haythem96 we don't have an explicit indentation rule, yet. > You can add `'indent': ['error', 2]` to the root `.eslintrc.js` in the rules section. Cool, fixed now.

👍 3 🚀 1

fsmanuel commented

2019-07-08 17:12:35 +00:00

(Migrated from github.com)

@haythem96 thanks for adding the rule. Now you got us:

lib/serializers/contributor.js
  16:1  error  Expected indentation of 2 spaces but found 1  indent

Can you please fix it! ❤️

@haythem96 thanks for adding the rule. Now you got us: ``` lib/serializers/contributor.js 16:1 error Expected indentation of 2 spaces but found 1 indent ``` Can you please fix it! ❤️

haythem96 commented

2019-07-09 00:04:28 +00:00

(Migrated from github.com)

@haythem96 thanks for adding the rule. Now you got us:
lib/serializers/contributor.js
  16:1  error  Expected indentation of 2 spaces but found 1  indent
Can you please fix it!

done.

> @haythem96 thanks for adding the rule. Now you got us: > > ``` > lib/serializers/contributor.js > 16:1 error Expected indentation of 2 spaces but found 1 indent > ``` > > Can you please fix it! done.

👍 1

raucao commented

2019-07-09 09:02:07 +00:00

(Migrated from github.com)

@haythem96 Did @bumi tell you about the kredits labels for issues and pull requests? This one is still missing the label...

(Documenting how this works should be solved during the current milestone, which is about onboarding.)

@haythem96 Did @bumi tell you about the kredits labels for issues and pull requests? This one is still missing the label... (Documenting how this works should be solved during the current milestone, which is about onboarding.)

haythem96 commented

2019-07-09 09:04:21 +00:00

(Migrated from github.com)

@haythem96 Did @bumi tell you about the kredits labels for issues and pull requests? This one is still missing the label...

(Documenting how this works should be solved during the current milestone, which is about onboarding.)

Yeah I know, I can't add a label...

> @haythem96 Did @bumi tell you about the kredits labels for issues and pull requests? This one is still missing the label... > > (Documenting how this works should be solved during the current milestone, which is about onboarding.) Yeah I know, I can't add a label...

raucao commented

2019-07-09 12:46:42 +00:00

(Migrated from github.com)

@bumi Could you make sure that @haythem96 is fully set up, please? Thanks!

bumi (Migrated from github.com) approved these changes 2019-07-10 08:12:49 +00:00

raucao commented

2019-07-10 08:51:40 +00:00

(Migrated from github.com)

The package lockfile is missing in this branch again. :/

raucao (Migrated from github.com) requested changes 2019-07-10 08:54:37 +00:00

raucao (Migrated from github.com) left a comment

I just pulled this branch and ran npm run test:contributor, which I assume is what runs the code in here.

Unfortunately, it just hangs with no output, after printing "Passing the command to Truffle":

I just pulled this branch and ran `npm run test:contributor`, which I assume is what runs the code in here. Unfortunately, it just hangs with no output, after printing "Passing the command to Truffle": ![Screenshot from 2019-07-10 10-54-24](https://user-images.githubusercontent.com/842/60955422-17236080-a301-11e9-8256-f981163f157e.png)

raucao commented

2019-07-10 09:05:13 +00:00

(Migrated from github.com)

@haythem96 I opened a PR to your repo/branch, so the tests are run on Travis: https://github.com/haythem96/kredits-contracts/pull/1

👍 1

bumi commented

2019-07-10 09:07:00 +00:00

(Migrated from github.com)

I've looked at the ganache-cli.sh script again and have a view questions/comments:

do we want to use npx there? all executables (aragon/truffle/ganache-cli) should be available as npm dependency anyway.
can we use aragon --devchain instead of running ganache-cli directly?
we don't have the rpc environment - we should probably add a test environment in the truffle.js

I've looked at the `ganache-cli.sh` script again and have a view questions/comments: * do we want to use npx there? all executables (aragon/truffle/ganache-cli) should be available as npm dependency anyway. * can we use `aragon --devchain` instead of running `ganache-cli` directly? * we don't have the `rpc` environment - we should probably add a `test` environment in the truffle.js

raucao commented

2019-07-10 09:19:23 +00:00

(Migrated from github.com)

Looks like Travis can't deal with merging PRs in repo forks: https://travis-ci.org/67P/kredits-contracts/pull_requests

I guess pushing one more commit should do the trick.

Looks like Travis can't deal with merging PRs in repo forks: https://travis-ci.org/67P/kredits-contracts/pull_requests I guess pushing one more commit should do the trick.

raucao commented

2019-07-10 09:26:51 +00:00

(Migrated from github.com)

If the other tests aren't supposed to work yet, then let's just change their npm scripts so that they don't run in npm test yet (e.g. change test:token to something like echo 'WIP' or similar).

If the other tests aren't supposed to work yet, then let's just change their npm scripts so that they don't run in `npm test` yet (e.g. change `test:token` to something like `echo 'WIP'` or similar).

haythem96 commented

2019-07-10 09:36:33 +00:00

(Migrated from github.com)

I just pulled this branch and ran npm run test:contributor, which I assume is what runs the code in here.

Unfortunately, it just hangs with no output, after printing "Passing the command to Truffle":

For some reasons if we change the env PORT variable and the port of the development network to 8545 it work...! cc @bumi

> I just pulled this branch and ran `npm run test:contributor`, which I assume is what runs the code in here. > > Unfortunately, it just hangs with no output, after printing "Passing the command to Truffle": > > ![Screenshot from 2019-07-10 10-54-24](https://user-images.githubusercontent.com/842/60955422-17236080-a301-11e9-8256-f981163f157e.png) For some reasons if we change the env PORT variable and the port of the development network to `8545` it work...! cc @bumi

haythem96 commented

2019-07-10 09:38:37 +00:00

(Migrated from github.com)

If the other tests aren't supposed to work yet, then let's just change their npm scripts so that they don't run in npm test yet (e.g. change test:token to something like echo 'WIP' or similar).

just modified the npm test scripts to use the the ganache-cli script in the project instead of the one in @aragon/test-helpers so the other tests should work fine also...

> If the other tests aren't supposed to work yet, then let's just change their npm scripts so that they don't run in `npm test` yet (e.g. change `test:token` to something like `echo 'WIP'` or similar). just modified the npm test scripts to use the the ganache-cli script in the project instead of the one in @aragon/test-helpers so the other tests should work fine also...

haythem96 commented

2019-07-10 09:40:20 +00:00

(Migrated from github.com)

I've looked at the ganache-cli.sh script again and have a view questions/comments:

do we want to use npx there? all executables (aragon/truffle/ganache-cli) should be available as npm dependency anyway.

can we use aragon --devchain instead of running ganache-cli directly?

we don't have the rpc environment - we should probably add a test environment in the truffle.js

So should we drop that ganache-cli script and use the devchain ?

> I've looked at the `ganache-cli.sh` script again and have a view questions/comments: > > * do we want to use npx there? all executables (aragon/truffle/ganache-cli) should be available as npm dependency anyway. > * can we use `aragon --devchain` instead of running `ganache-cli` directly? > * we don't have the `rpc` environment - we should probably add a `test` environment in the truffle.js So should we drop that ganache-cli script and use the devchain ?

bumi commented

2019-07-10 09:48:09 +00:00

(Migrated from github.com)

@haythem96 no idea :) I was just wondering. As aragon devchain is also using ganache-cli I thought that might be closer to the dev environment.
what do you think?

@haythem96 no idea :) I was just wondering. As `aragon devchain` is also using ganache-cli I thought that might be closer to the dev environment. what do you think?

😕 1

haythem96 commented

2019-07-10 10:11:12 +00:00

(Migrated from github.com)

the tests should work now with running devchain manually before npm run test,
didn't know how to config travis this way XD ...

the tests should work now with running devchain manually before `npm run test`, didn't know how to config travis this way XD ...

haythem96 commented

2019-07-16 11:51:49 +00:00

(Migrated from github.com)

I want more Kredits token :p can we fix the travis config ? the tests work now when running the devchain

I want more Kredits token :p can we fix the travis config ? the tests work now when running the `devchain`

raucao commented

2019-07-16 11:56:56 +00:00

(Migrated from github.com)

What exactly is unclear about doing the same on Travis as you do locally? Maybe I can answer your question.

These docs might help: https://docs.travis-ci.com/user/job-lifecycle/

What exactly is unclear about doing the same on Travis as you do locally? Maybe I can answer your question. These docs might help: https://docs.travis-ci.com/user/job-lifecycle/

👍 1

haythem96 commented

2019-07-18 08:32:48 +00:00

(Migrated from github.com)

I think Travis build works now

bumi (Migrated from github.com) requested changes 2019-07-18 15:23:28 +00:00

bumi (Migrated from github.com) left a comment

looks good, travis is happy... I got two more comments and I think we are good to merge :shipit:
what do you say? @skddc

looks good, travis is happy... I got two more comments and I think we are good to merge :shipit: what do you say? @skddc

apps/contributor/package.json

bumi (Migrated from github.com) commented

2019-07-18 15:21:56 +00:00

do we still need those? and if so should we use the latest version?

package.json

bumi (Migrated from github.com) commented

2019-07-18 15:22:31 +00:00

same here: do we still need this and if so should we use the latest version?

haythem96 (Migrated from github.com) reviewed 2019-07-18 15:26:16 +00:00

apps/contributor/package.json

haythem96 (Migrated from github.com) commented

2019-07-18 15:26:16 +00:00

I used the test-helpers package in the tests. For the solidity-coverage we may need it... or should I remove it ?

bumi (Migrated from github.com) reviewed 2019-07-18 15:34:53 +00:00

apps/contributor/package.json

bumi (Migrated from github.com) commented

2019-07-18 15:34:53 +00:00

no, I was only wondering about the test-helpers. can we use the latest version here? 2.0.0
https://www.npmjs.com/package/@aragon/test-helpers or does that beak things?

no, I was only wondering about the test-helpers. can we use the latest version here? 2.0.0 https://www.npmjs.com/package/@aragon/test-helpers or does that beak things?

haythem96 (Migrated from github.com) reviewed 2019-07-20 01:33:12 +00:00

apps/contributor/package.json

haythem96 (Migrated from github.com) commented

2019-07-20 01:33:12 +00:00

I updated it now, did not break anything :)

bumi (Migrated from github.com) approved these changes 2019-07-20 07:24:11 +00:00

bumi (Migrated from github.com) left a comment

ok, looks great, thanks a lot.
let's wait for @skddc's approval and then we merge it.

ok, looks great, thanks a lot. let's wait for @skddc's approval and then we merge it.

raucao commented

2019-07-20 09:04:22 +00:00

(Migrated from github.com)

I got two more comments and I think we are good to merge :shipit:
what do you say? @skddc

I trust your opinion. But please clean up the git history when merging. ;)

> I got two more comments and I think we are good to merge :shipit: > what do you say? @skddc I trust your opinion. But please clean up the git history when merging. ;)

bumi commented

2019-07-20 09:16:46 +00:00

(Migrated from github.com)

ok, cool. if we find issues or optimizations we do that in a new PR
I'd say we simply squasch merge this one.

ok, cool. if we find issues or optimizations we do that in a new PR I'd say we simply squasch merge this one.

Sign in to join this conversation.

No reviewers

raucao

bumi