Refactor: Update test-helpers to web3 1.0 and include new utils #30

0xGabi · 2020-04-11T02:11:13Z

Note: On the last commit 152f97a I applied linting to the repo

0xGabi · 2020-04-13T20:10:36Z

packages/test-helpers/asserts/assertBn.js

@@ -0,0 +1,7 @@
+const assertBn = (actual, expected, errorMsg) => {


Move asserts under the same path.

0xGabi · 2020-04-13T20:11:22Z

packages/test-helpers/constants.js

@@ -0,0 +1,2 @@
+export const ANY_ADDRESS = '0xffffffffffffffffffffffffffffffffffffffff'


Include the two constant used very frequently. Do you think there are others that make sense?

I'm wondering if we shouldn't have an "ACL" scope for some of these utilities, to include the "any" and "burned" addresses.

Maybe a contract-helpers-os package or similar. We're starting to mix some generic solidity testing utilities with utilities specifically designed to be used on aragonOS.

0xGabi · 2020-04-13T20:11:47Z

packages/test-helpers/events.js

@@ -8,8 +8,10 @@ const getEventArgument = (receipt, event, arg, index = 0) =>
  getEventAt(receipt, event, index).args[arg]
 const getNewProxyAddress = receipt =>
  getEventArgument(receipt, 'NewAppProxy', 'proxy')
+  const getNewDaoAddress = receipt =>


Include a helper to get the DeployDAO event.

0xGabi · 2020-04-13T20:18:27Z

packages/test-helpers/ganache-cli.sh

-  sleep 3
-  echo "Running testrpc-sc with pid ${rpc_pid} in port ${PORT}"
-}
+  while ! rpc_running; do


Add logic to test rpc connection is open using Netcat

0xGabi · 2020-04-13T20:19:05Z

packages/test-helpers/ganache-cli.sh

-  start_testrpc
-  measure_coverage $@
+  start_ganache
+  node_modules/.bin/buidler coverage --network coverage "$@"


Use buidler

Maybe we can specify these bits as arguments to this script. As it is, we're forcing users to always use buidler.

This is not necessarily bad, but the way I see this script is that it sets up the local ganache environment to run a test or coverage script, whichever script that is.

0xGabi · 2020-04-13T20:19:45Z

packages/test-helpers/lib/aragon.js

@@ -1,9 +1,10 @@
 const abi = require('ethereumjs-abi')
-
-const createExecutorId = id => `0x${String(id).padStart(8, '0')}`
+const { bn } = require('./lib/numbers')


Put all Aragon helpers in a single file

Ah I mentioned above, but I've been thinking of moving this to a separate package altogether so it's more clear.

We have a lot more helpers than just the ones listed here that are only relevant to aragonOS.

0xGabi · 2020-04-13T20:20:22Z

packages/test-helpers/lib/namehash.js

@@ -0,0 +1,5 @@
+const { hash: namehash } = require('eth-ens-namehash')


Expose namehash as helper.

0xGabi · 2020-04-13T20:21:37Z

packages/test-helpers/package.json

@@ -1,20 +1,24 @@
 {
-  "name": "@aragon/contract-helpers-test",
-  "version": "0.0.2",
+  "name": "@aragon/test-helpers",


I finally choose to keep using the same test-helpers namespace. And maybe we do a major release?

I've been thinking that test-helpers is a bit too generic, since we have frontend and contract testing.

Maybe we can publish under @aragon/contract-test-helpers? And in the future we can have @aragon/fe-test-helpers (or similar)?

0xGabi · 2020-04-13T20:23:52Z

packages/test-helpers/package.json

-    "solidity-coverage": "^0.6.2"
+    "ganache-cli": "^6.9.1",
+    "solidity-coverage": "^0.7.4",
+    "web3": "^1.0.0"


I included web3 as peerDependency to force to update from 0.2.x

0xGabi · 2020-04-13T20:26:18Z

packages/test-helpers/lib/web3.js

@@ -0,0 +1,130 @@
+module.exports = web3 => {


I decided to use a single web3 helper file as we have in aragonOS. What do you think?

.eslintignore

.prettierignore

sohkai · 2020-04-14T11:44:22Z

.eslintrc

@@ -0,0 +1,23 @@
+{
+  "env": {
+    "browser": true,


We most likely shouldn't set browser to be true—this is primarily for a node environment.

.eslintrc

sohkai · 2020-04-14T11:48:33Z

packages/test-helpers/constants.js

@@ -0,0 +1,2 @@
+export const ANY_ADDRESS = '0xffffffffffffffffffffffffffffffffffffffff'


I'm wondering if we shouldn't have an "ACL" scope for some of these utilities, to include the "any" and "burned" addresses.

Maybe a contract-helpers-os package or similar. We're starting to mix some generic solidity testing utilities with utilities specifically designed to be used on aragonOS.

sohkai · 2020-04-14T12:03:48Z

packages/test-helpers/lib/web3.js

+      method: 'evm_mine',
+      id: new Date().getTime()
+    })
+    return web3.eth.getBlock('latest').hash


We almost definitely need to use an await on getBlock() first before accessing hash

Ouch! Good catch

sohkai · 2020-04-14T12:05:02Z

packages/test-helpers/lib/web3.js

+    return web3.eth.getBlock('latest')
+  }
+
+  const advanceTimeAndBlocksBy = async (time, numOfBlocks) => {


Are these time/block controlling utilities used by the court tests? We should usually shy away from these types of utilities, because they aren't usable in an environment outside of ganache.

If they are, I would still push for them to be scoped in a different namespace, e.g. time-control

Yes, see for instance: https://github.com/aragon/aragon-court/blob/master/test/helpers/wrappers/court.js#L66 and https://github.com/aragon/aragon-court/blob/master/test/helpers/lib/blocks.js

Made an issue to add these back in! #46

sohkai · 2020-04-14T12:06:29Z

packages/test-helpers/lib/web3.js

+    getBlockNumber,
+    getBlock,
+    sendTransaction,
+    getTransaction


Let's alphabetize :)

sohkai · 2020-04-14T12:10:00Z

packages/test-helpers/lib/web3.js

+    return web3.eth.getBlock('latest')
+  }
+
+  function signatures(contract, exclude, web3, names = false, excludeConstant = false) {


I'm not super sure this util is relevant as a general util, and this type of thing is usually already done by the contract library that's being used (via an encoding function or otherwise).

What do you think about removing it?

sohkai · 2020-04-14T12:11:20Z

packages/test-helpers/lib/web3.js

+    const signedPayload = await web3.eth.sign(address, payload)
+    const adding0x = x => '0x'.concat(x)
+    return {
+      r: adding0x(signedPayload.substr(2, 64)),


Some comments here would be really useful!

I'm also not sure if the way we have this is fully generic and usable by scenarios where we need to create these signature fields, but good to ask Facu on this.

We may also want to come up with a different name, e.g. deriveSignatureRSV() or etc.

sohkai · 2020-04-14T12:14:26Z

packages/test-helpers/ganache-cli.sh

-  GAS_LIMIT=${GAS_LIMIT-8000000}
-  NETWORK_ID=${NETWORK_ID-15}
-  ACCOUNTS=${ACCOUNTS-200}
+rpc_running() {


There are two rpc_running definitions now

sohkai · 2020-04-14T12:15:19Z

packages/test-helpers/ganache-cli.sh

-  rpc_pid=$!
-  echo "Waiting for ganache to launch on port "$PORT"..."
+  if [ "$SOLIDITY_COVERAGE" = true ]; then
+    export RUNNING_COVERAGE=true


Interesting, do we not need to set up the coverage-testrpc if we're using builder?

sohkai · 2020-07-14T18:40:27Z

packages/test-helpers/lib/web3.js

@@ -0,0 +1,134 @@
+module.exports = (web3) => {


I ended up not including most of these in #43.

I'm open to adding them—let's confirm a use case first!

sohkai · 2020-07-14T18:41:20Z

Closing for #43.

Refactor: Update test-helpers to web3 1.0 and include new utils

f64d20a

0xGabi requested a review from sohkai April 11, 2020 02:11

Polish final details and include web3 1.x as peerDependency

da05e03

0xGabi marked this pull request as ready for review April 13, 2020 20:09

0xGabi commented Apr 13, 2020

View reviewed changes

0xGabi added 4 commits April 13, 2020 17:34

Remove reverString and move makeErrorMapping to utils

c0ac7a6

Fix: wrong assignment

cfd423b

Lint: add linting and applied

152f97a

Fix: move to buidler compatible ganache-cli script

6446b4e

0xGabi mentioned this pull request Apr 14, 2020

Finance: Move to buidler setup aragon/aragon-apps#1109

Merged

sohkai reviewed Apr 14, 2020

View reviewed changes

This was referenced Jul 13, 2020

[BREAKING] Test: Organize, improve and provide new helpers #35

Closed

test-helpers: re-organize, update to web3 1.0, and add utilities from dao-templates, react-boilerplate #43

Merged

sohkai reviewed Jul 14, 2020

View reviewed changes

sohkai closed this Jul 14, 2020

sohkai deleted the refactor-tests-helpers branch July 14, 2020 18:41

sohkai mentioned this pull request Jul 15, 2020

test-helpers: add time-control utilities #46

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Update test-helpers to web3 1.0 and include new utils #30

Refactor: Update test-helpers to web3 1.0 and include new utils #30

0xGabi commented Apr 11, 2020 •

edited

Loading

0xGabi Apr 13, 2020

0xGabi Apr 13, 2020

sohkai Apr 14, 2020

0xGabi Apr 13, 2020

0xGabi Apr 13, 2020 •

edited

Loading

0xGabi Apr 13, 2020

sohkai Apr 14, 2020

0xGabi Apr 13, 2020 •

edited

Loading

sohkai Apr 14, 2020

0xGabi Apr 13, 2020 •

edited

Loading

0xGabi Apr 13, 2020 •

edited

Loading

sohkai Apr 14, 2020

0xGabi Apr 13, 2020 •

edited

Loading

0xGabi Apr 13, 2020 •

edited

Loading

sohkai Apr 14, 2020

sohkai Apr 14, 2020

sohkai Apr 14, 2020

0xGabi Apr 15, 2020

sohkai Apr 14, 2020

bingen Jul 15, 2020

sohkai Jul 15, 2020

sohkai Apr 14, 2020

sohkai Apr 14, 2020

sohkai Apr 14, 2020

sohkai Apr 14, 2020

sohkai Apr 14, 2020

sohkai Jul 14, 2020

sohkai commented Jul 14, 2020

		@@ -0,0 +1,7 @@
		const assertBn = (actual, expected, errorMsg) => {

		@@ -0,0 +1,2 @@
		export const ANY_ADDRESS = '0xffffffffffffffffffffffffffffffffffffffff'

		@@ -0,0 +1,5 @@
		const { hash: namehash } = require('eth-ens-namehash')

Refactor: Update test-helpers to web3 1.0 and include new utils #30

Refactor: Update test-helpers to web3 1.0 and include new utils #30

Conversation

0xGabi commented Apr 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xGabi Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xGabi Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xGabi Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

0xGabi Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xGabi Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

0xGabi Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Jul 14, 2020

0xGabi commented Apr 11, 2020 •

edited

Loading

0xGabi Apr 13, 2020 •

edited

Loading

0xGabi Apr 13, 2020 •

edited

Loading

0xGabi Apr 13, 2020 •

edited

Loading

0xGabi Apr 13, 2020 •

edited

Loading

0xGabi Apr 13, 2020 •

edited

Loading

0xGabi Apr 13, 2020 •

edited

Loading