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

Add more integration tests #952

Merged
merged 20 commits into from
Nov 26, 2019
Merged

Conversation

dapplion
Copy link
Contributor

@dapplion dapplion commented Nov 23, 2019

Add tests to increase the code coverage of the /lib directory.

As of 9e3bb5b it increases the coverage of /lib by +32.68% to 73.64%.

Issues on tested code:

Tests never finish on error ✔️

travis - npm - ava - nyc or someone in the chain seems to swallow the error when it fails, forcing the test to take extra 10 minutes for the Travis timeout to kick in.

I hadn't the time to investigate the cause yet.

Issues to-be-tested code (or preventing it from being tested):

No snapshot diffs ⌛

AVA does not show a diff of snapshot from Travis CI. See example https://travis-ci.org/aragon/aragon-cli/jobs/616072516#L486

acl view ✔️

This test always fails since ava thinks that the promise wrapping AragonJS never resolves, and fails with:

✖ Should get formated permissions for a dao apps
Promise returned by test never resolved

Trying different async constructions didn't solve the issue. Therefore the test is skipped until there's more time to find a solution

start ⌛

According to #858 the start commands are not yet refactored. However there's code in /lib but the code if highly coupled with the front-end calling task.skip() and mutating the task.ctx
@0xGabi, is this refactored? Is this a WIP?

@dapplion dapplion changed the title Integration tests WIP Integration tests Nov 23, 2019
@codecov
Copy link

codecov bot commented Nov 24, 2019

Codecov Report

Merging #952 into develop will increase coverage by 4.63%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #952      +/-   ##
===========================================
+ Coverage    15.25%   19.89%   +4.63%     
===========================================
  Files          104      104              
  Lines         2523     2523              
===========================================
+ Hits           385      502     +117     
+ Misses        2138     2021     -117
Impacted Files Coverage Δ
packages/aragon-cli/src/util.js 49.67% <0%> (+3.92%) ⬆️
...agon-cli/src/lib/apm/grantNewVersionsPermission.js 100% <0%> (+4.16%) ⬆️
packages/aragon-cli/src/lib/ipfs/config.js 23.8% <0%> (+4.76%) ⬆️
...ackages/aragon-cli/src/helpers/aragonjs-wrapper.js 69.44% <0%> (+5.55%) ⬆️
packages/aragon-cli/src/lib/ipfs/daemon.js 8.16% <0%> (+8.16%) ⬆️
...ckages/aragon-cli/src/lib/start/download-client.js 20% <0%> (+20%) ⬆️
packages/aragon-cli/src/lib/ipfs/misc.js 40% <0%> (+40%) ⬆️
packages/aragon-cli/src/lib/deploy.js 90% <0%> (+50%) ⬆️
packages/aragon-cli/src/lib/acl/view.js 86.66% <0%> (+86.66%) ⬆️
packages/aragon-cli/src/lib/ipfs/propagation.js 88.88% <0%> (+88.88%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8dc2c36...398895e. Read the comment docs.

@dapplion dapplion force-pushed the integration_tests branch 4 times, most recently from 1a459f5 to 61690dd Compare November 24, 2019 17:17
Install IPFS in Travis environment

f

Add sudo to IPFS installation

Fix IPFS installation command

Add util

init IPFS after install

Test also isIPFSRunning
@dapplion dapplion changed the title WIP Integration tests Add more integration tests Nov 24, 2019
@dapplion dapplion mentioned this pull request Nov 24, 2019
4 tasks
@0xGabi 0xGabi mentioned this pull request Nov 24, 2019
43 tasks
@0xGabi
Copy link
Contributor

0xGabi commented Nov 25, 2019

@dapplion The start command went the first command to be refactored a long time ago. I didn't tackle it yet, so if it is blocking you to keep testing feel free to take it

@macor161 macor161 self-requested a review November 25, 2019 03:55
@macor161
Copy link
Contributor

I'll do the start command if you haven't already began working on it @dapplion

@dapplion
Copy link
Contributor Author

I'll do the start command if you haven't already began working on it @dapplion

I can take it, which will ease testing it for me

@0xGabi 0xGabi added the SDK v7 label Nov 25, 2019
@kernelwhisperer
Copy link
Contributor

acl view
This test always fails since ava thinks that the promise wrapping AragonJS never resolves, and fails with:
✖ Should get formated permissions for a dao apps
Promise returned by test never resolved
Trying different async constructions didn't solve the issue. Therefore the test is skipped until there's more time to find a solution

Looks like the problem in our getLocalWeb3 test utility:

- new Web3.providers.HttpProvider(`http://localhost:8545`)
+ new Web3.providers.WebsocketProvider(`ws://localhost:8545`)

Because it was connecting using an http provider, the wrapper.apps observable emitted only once with ['Kernel'] and considering how getApps works this promise would never finish.

@kernelwhisperer
Copy link
Contributor

kernelwhisperer commented Nov 26, 2019

Tests never finish on error
travis - npm - ava - nyc or someone in the chain seems to swallow the error when it fails, forcing the test to take extra 10 minutes for the Travis timeout to kick in.

The issue seems to be that the devchain remains started and lerna is waiting for it to exit, which never happens.

It seems to be that this is the "correct" npm behavior.

I found a solution in d76491a inspired by this SO answer which forces the postscript to run even if the tests have failed and return the correct exit code to make the CI fail as it should.

Update: I had to update the script once more in af2d676 because without the parenthesis && exit 1 was evaluated on success too.

Copy link
Contributor

@kernelwhisperer kernelwhisperer left a comment

Choose a reason for hiding this comment

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

LGTM!

I've skipped some ipfs tests and moved the concerns you shared in #951 so that we can merge this.

Regarding the snapshots problem I'm not sure how to replicate it, my guess is that lerna hanging or some other issue was causing it now to show.

I've tested in 8ebe5b8 and it seems to work: https://travis-ci.org/aragon/aragon-cli/jobs/617278237#L841

@dapplion
Copy link
Contributor Author

Awesome, thanks for catching the issue

Copy link
Contributor

@macor161 macor161 left a comment

Choose a reason for hiding this comment

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

Awesome work on test coverage increase @dapplion and @0x6431346e ! 💯
@0x6431346e will experiment in a second iteration on how to improve IPFS-related integration tests.

@macor161 macor161 merged commit 2619328 into aragon:develop Nov 26, 2019
@dapplion dapplion deleted the integration_tests branch November 28, 2019 23:45
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.

None yet

4 participants