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

WIP: Contribution contracts tests #159

Closed
haythem96 wants to merge 20 commits from tests/contracts-contribution into master
pull from: tests/contracts-contribution
merge into: :master
Conversation 30 Commits 20 Files Changed 12
+14583 -2493
haythem96 commented 2019-08-02 16:13:42 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Related to #103

Related to #103
haythem96 (Migrated from github.com) reviewed 2019-08-02 16:29:20 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-02 16:29:19 +00:00
Copy Link
Copy Source

this one revert for some reasons

this one revert for some reasons
raucao (Migrated from github.com) reviewed 2019-08-02 16:39:17 +00:00
apps/contribution/test/contribution.js
raucao (Migrated from github.com) commented 2019-08-02 16:39:17 +00:00
Copy Link
Copy Source

Looks like the expectation wasn't described here (it("does x")). It also looks like there are 3 different expectations in one block here.

Looks like the expectation wasn't described here (`it("does x")`). It also looks like there are 3 different expectations in one block here.
raucao (Migrated from github.com) reviewed 2019-08-02 16:40:09 +00:00
apps/contribution/test/contribution.js
raucao (Migrated from github.com) commented 2019-08-02 16:40:09 +00:00
Copy Link
Copy Source

I think you can also add expectation descriptions as the last argument to assert(), in case you really want to assert multiple things in one it block.

I think you can also add expectation descriptions as the last argument to `assert()`, in case you really want to assert multiple things in one `it` block.
raucao (Migrated from github.com) reviewed 2019-08-02 16:40:45 +00:00
apps/contribution/test/contribution.js
raucao (Migrated from github.com) commented 2019-08-02 16:40:45 +00:00
Copy Link
Copy Source

Reverts unexpectedly? And in which context/scenario?

Reverts unexpectedly? And in which context/scenario?
haythem96 (Migrated from github.com) reviewed 2019-08-03 11:24:35 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-03 11:24:35 +00:00
Copy Link
Copy Source

in the scenario of claiming an added contribution

in the scenario of claiming an added contribution
haythem96 (Migrated from github.com) reviewed 2019-08-03 11:37:47 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-03 11:37:47 +00:00
Copy Link
Copy Source

most of those multiple assert check for the same thing, that the contribution is added.. XD

most of those multiple assert check for the same thing, that the contribution is added.. XD
raucao (Migrated from github.com) reviewed 2019-08-03 11:58:16 +00:00
apps/contribution/test/contribution.js
raucao (Migrated from github.com) commented 2019-08-03 11:58:16 +00:00
Copy Link
Copy Source

How would that work? You can only claim confirmed contributions.

How would that work? You can only claim confirmed contributions.
raucao (Migrated from github.com) reviewed 2019-08-03 11:59:37 +00:00
apps/contribution/test/contribution.js
raucao (Migrated from github.com) commented 2019-08-03 11:59:37 +00:00
Copy Link
Copy Source

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.

They very line I commented on asserts that the contributorId is correct, not that the contribution itself was added.
haythem96 (Migrated from github.com) reviewed 2019-08-03 14:40:57 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-03 14:40:57 +00:00
Copy Link
Copy Source

ahhhh yup okay, missed this require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); thanks!

ahhhh yup okay, missed this ` require(block.number >= c.confirmedAtBlock, 'NOT_CLAIMABLE'); ` thanks!
haythem96 (Migrated from github.com) reviewed 2019-08-03 14:42:56 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-03 14:42:56 +00:00
Copy Link
Copy Source

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage

yeah true, honestly don't know if that all those asserts are necessary... maybe just for more code coverage
raucao (Migrated from github.com) reviewed 2019-08-03 14:46:37 +00:00
apps/contribution/test/contribution.js
raucao (Migrated from github.com) commented 2019-08-03 14:46:37 +00:00
Copy Link
Copy Source

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.

In this case it's only important that you describe what you mean to test exactly. Currently, there's no description of what is being tested in this block.
haythem96 (Migrated from github.com) reviewed 2019-08-06 10:37:30 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-06 10:37:30 +00:00
Copy Link
Copy Source

this should fix the claim tests, no @skddc @bumi ?

this should fix the claim tests, no @skddc @bumi ?
haythem96 (Migrated from github.com) reviewed 2019-08-09 22:54:48 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-09 22:54:48 +00:00
Copy Link
Copy Source

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi

idk if this test is really necessary as if a contribution is claimed, so theoretically it is impossible to veto because of block number @bumi
haythem96 (Migrated from github.com) reviewed 2019-08-09 22:55:23 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-09 22:55:23 +00:00
Copy Link
Copy Source

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc

can't figure out why this one is not working, idk what I'm missing there! @bumi @skddc
haythem96 (Migrated from github.com) reviewed 2019-08-09 22:56:08 +00:00
apps/contribution/test/contribution.js
haythem96 (Migrated from github.com) commented 2019-08-09 22:56:07 +00:00
Copy Link
Copy Source

blocks get minted and the veto period end right away... @bumi @skddc

blocks get minted and the veto period end right away... @bumi @skddc
raucao (Migrated from github.com) reviewed 2019-08-10 10:51:39 +00:00
apps/contribution/test/contribution.js
@@ -0,0 +87,4 @@
appsId[3] = namehash("kredits-token");
//get new app instance from DAO
let receipt = await dao.newAppInstance(
raucao (Migrated from github.com) commented 2019-08-10 10:51:38 +00:00
Copy Link
Copy Source

getBlockNumber should actually return a promise as well, so this entire function is superfluous. We're using it with a promise e.g. here: https://github.com/67P/kredits-web/blob/master/app/services/kredits.js#L150

`getBlockNumber` should actually return a promise as well, so this entire function is superfluous. We're using it with a promise e.g. here: https://github.com/67P/kredits-web/blob/master/app/services/kredits.js#L150
haythem96 (Migrated from github.com) reviewed 2019-08-10 13:40:39 +00:00
apps/contribution/test/contribution.js
@@ -0,0 +87,4 @@
appsId[3] = namehash("kredits-token");
//get new app instance from DAO
let receipt = await dao.newAppInstance(
haythem96 (Migrated from github.com) commented 2019-08-10 13:40:39 +00:00
Copy Link
Copy Source

it return also there a promise, no ?

it return also there a promise, no ?
raucao (Migrated from github.com) reviewed 2019-08-10 13:50:41 +00:00
apps/contribution/test/contribution.js
@@ -0,0 +87,4 @@
appsId[3] = namehash("kredits-token");
//get new app instance from DAO
let receipt = await dao.newAppInstance(
raucao (Migrated from github.com) commented 2019-08-10 13:50:41 +00:00
Copy Link
Copy Source

The function you added is just wrapping the normal callback in a promise. Wherever it's called you can just use the original function with a promise.

The function you added is just wrapping the normal callback in a promise. Wherever it's called you can just use the original function with a promise.
haythem96 (Migrated from github.com) reviewed 2019-08-10 14:36:36 +00:00
apps/contribution/test/contribution.js
@@ -0,0 +87,4 @@
appsId[3] = namehash("kredits-token");
//get new app instance from DAO
let receipt = await dao.newAppInstance(
haythem96 (Migrated from github.com) commented 2019-08-10 14:36:36 +00:00
Copy Link
Copy Source

I updated it, the thing is blocks get mined instantly so veto period end...

I updated it, the thing is blocks get mined instantly so veto period end...
raucao (Migrated from github.com) reviewed 2019-09-19 09:19:33 +00:00
apps/contribution/test/contribution.js
raucao (Migrated from github.com) commented 2019-09-19 09:19:32 +00:00
Copy Link
Copy Source

Yes, the first 10 are confirmed immediately iirc.

Yes, the first 10 are confirmed immediately iirc.
haythem96 commented 2019-09-19 15:36:47 +00:00 (Migrated from github.com)
Copy Link
Copy Source

As an update:

  • still have one error with the claim contribution test.
  • for travis, even when tests fail, it exit with 0!
As an update: - still have one error with the claim contribution test. - for travis, even when tests fail, it exit with 0!
raucao (Migrated from github.com) reviewed 2019-09-21 07:37:44 +00:00
.travis.yml
@@ -18,0 +22,4 @@
- TASK=test:contributor
- TASK=test:contribution
- TASK=test:proposal
raucao (Migrated from github.com) commented 2019-09-21 07:37:44 +00:00
Copy Link
Copy Source

This should be the same as the test script itself, because && is supposed to exit with 1, if any of the commands on the way exit with 1.

This should be the same as the test script itself, because `&&` is supposed to exit with 1, if any of the commands on the way exit with 1.
haythem96 (Migrated from github.com) reviewed 2019-09-21 12:35:10 +00:00
.travis.yml
@@ -18,0 +22,4 @@
- TASK=test:contributor
- TASK=test:contribution
- TASK=test:proposal
haythem96 (Migrated from github.com) commented 2019-09-21 12:35:10 +00:00
Copy Link
Copy Source

yeah true, I changed that also to

env:
  - TASK=lint:wrapper
  - TASK=lint:contract-tests
  - TASK=test:token
  - TASK=test:contributor
  - TASK=test:contribution
  - TASK=test:proposal

before_script:
  - npm run devchain > /dev/null &
  - sleep 5

script:
  - travis_wait 60 npm run $TASK

It is the same also, but > /dev/null & to not show the output and using TASK for better visualization I think...
But the thing is there is an error in Contribution tests, but it doesn't exit with 1!

yeah true, I changed that also to ``` env: - TASK=lint:wrapper - TASK=lint:contract-tests - TASK=test:token - TASK=test:contributor - TASK=test:contribution - TASK=test:proposal before_script: - npm run devchain > /dev/null & - sleep 5 script: - travis_wait 60 npm run $TASK ``` It is the same also, but `> /dev/null &` to not show the output and using `TASK` for better visualization I think... But the thing is there is an error in Contribution tests, but it doesn't exit with 1!
raucao (Migrated from github.com) reviewed 2019-09-22 08:46:26 +00:00
.travis.yml
@@ -18,0 +22,4 @@
- TASK=test:contributor
- TASK=test:contribution
- TASK=test:proposal
raucao (Migrated from github.com) commented 2019-09-22 08:46:26 +00:00
Copy Link
Copy Source

I don't see how the env vars make it clearer. I think it's actually less understandable that way. In fact, I wouldn't even know off the top of my head what happens when you assign the same env variable twice.

But the thing is there is an error in Contribution tests, but it doesn't exit with 1!

Yes, that's the thing that needs to be fixed I think. Not how the tests are run. The test task should just be npm test and nothing else, in my opinion.

I don't see how the env vars make it clearer. I think it's actually less understandable that way. In fact, I wouldn't even know off the top of my head what happens when you assign the same env variable twice. > But the thing is there is an error in Contribution tests, but it doesn't exit with 1! Yes, that's the thing that needs to be fixed I think. Not how the tests are run. The test task should just be `npm test` and nothing else, in my opinion.
haythem96 (Migrated from github.com) reviewed 2019-09-23 00:24:50 +00:00
.travis.yml
@@ -18,0 +22,4 @@
- TASK=test:contributor
- TASK=test:contribution
- TASK=test:proposal
haythem96 (Migrated from github.com) commented 2019-09-23 00:24:50 +00:00
Copy Link
Copy Source

What I meant by the env vars make it clearer, in Travis u don't need to look at the whole log in case a test fail...
1

What I meant by the env vars make it clearer, in Travis u don't need to look at the whole log in case a test fail... ![1](https://user-images.githubusercontent.com/17862704/65396697-eb146900-dda0-11e9-9186-2184cbf79106.png)
haythem96 (Migrated from github.com) reviewed 2019-09-23 00:29:17 +00:00
package.json
@@ -45,3 +45,4 @@
"@aragon/kits-base": "^1.0.0",
"@aragon/os": "^4.2.0",
"@aragon/test-helpers": "^2.0.0",
"async-each-series": "^1.1.0",
haythem96 (Migrated from github.com) commented 2019-09-23 00:29:17 +00:00
Copy Link
Copy Source

Adding the test-helpers package fixed #163

Adding the test-helpers package fixed #163
raucao (Migrated from github.com) reviewed 2019-09-23 10:01:00 +00:00
.travis.yml
@@ -18,0 +22,4 @@
- TASK=test:contributor
- TASK=test:contribution
- TASK=test:proposal
raucao (Migrated from github.com) commented 2019-09-23 10:01:00 +00:00
Copy Link
Copy Source

Oh, I see. The feature you're looking for is the build matrix:

https://docs.travis-ci.com/user/customizing-the-build/#naming-jobs-within-matrices

Oh, I see. The feature you're looking for is the build matrix: https://docs.travis-ci.com/user/customizing-the-build/#naming-jobs-within-matrices
haythem96 commented 2019-11-20 23:21:19 +00:00 (Migrated from github.com)
Copy Link
Copy Source

as an update for this PR; build run well... there is a revert in the claim contribution test, but test doesn't exit with exit code 1...

this call revert: address contributorAccount = getContributorAddressById(c.contributorId);

When I try to call any function in the Contribution app that call another app function, it revert.
like getContributorIdByAddress()

Am I missing something in the tests ?

as an update for this PR; build run well... there is a revert in the claim contribution test, but test doesn't exit with exit code 1... this call revert: ` address contributorAccount = getContributorAddressById(c.contributorId);` When I try to call any function in the Contribution app that call another app function, it revert. like `getContributorIdByAddress()` Am I missing something in the tests ?
raucao commented 2021-11-14 22:46:31 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Closing due to inactivity. Please feel free to re-open.

Closing due to inactivity. Please feel free to re-open.
raucao commented 2021-11-14 22:47:14 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Closing due to inactivity. Please feel free to re-open.

Closing due to inactivity. Please feel free to re-open.

Pull request closed

Please reopen this pull request to perform a merge.
This pull request is marked as a work in progress.
Sign in to join this conversation.
Reviewers
No Reviewers
haythem96
raucao
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#159