Refactor/split tests #102

satello · 2018-03-14T11:04:29Z

Split tests into different files (closes DRY out setup of integration tests and break them into smaller tests #90 )
Put all write requests into a queue in StoreProvider (closes Use a shared queue for every action that hits the store #94 )

Hopefully the tests will be more reliable now and quicker. Next step is to mock the store interactions. Using the hosted store slows the tests down 3x.

NOTE: when running the tests locally you will need 15 accounts on your testrpc client

coveralls · 2018-03-14T11:07:58Z

Pull Request Test Coverage Report for Build 516

81 of 85 (95.29%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+1.3%) to 71.849%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils/StoreProviderWrapper.js	77	78	98.72%
src/contractWrappers/ArbitrableTransactionWrapper.js	0	3	0.0%

Totals
Change from base Build 501:	1.3%
Covered Lines:	664
Relevant Lines:	846

💛 - Coveralls

…h 15 accounts

epiqueras

Huge improvement! Just a few changes needed.

epiqueras · 2018-03-14T19:51:19Z

.travis.yml

@@ -12,7 +12,7 @@ addons:
 install: yarn install --pure-lockfile
 script:
  - yarn run lint
-  - nohup yarn run ganache-cli &
+  - nohup yarn run ganache-cli -a 15 &


yarn run ganache since it is already a script.

epiqueras · 2018-03-14T19:51:51Z

package.json

@@ -28,7 +28,8 @@
    "commitmsg": "kleros-scripts commitmsg",
    "cz": "kleros-scripts cz",
    "start": "webpack --env.NODE_ENV=development --progress --watch",
-    "build": "webpack --env.NODE_ENV=production -p"
+    "build": "webpack --env.NODE_ENV=production -p",
+    "ganache": "ganache-cli -a 15"


This can go right before test to make the order clear.

epiqueras · 2018-03-14T19:52:03Z

src/abstractWrappers/ArbitrableContract.js

-      title,
-      description
+      contractInstance.address,
+      {


epiqueras · 2018-03-14T19:54:17Z

src/abstractWrappers/Arbitrator.js

-      const userProfile = await this._StoreProvider.setUpUserProfile(account)
-      // FIXME seems like a super hacky way to update store
-      userProfile.balance =
-        (parseInt(userProfile.balance, 10) ? userProfile.balance : 0) +


epiqueras · 2018-03-14T19:55:12Z

src/abstractWrappers/Disputes.js

          dispute.arbitratorAddress,
          dispute.disputeId,
-          (dispute.netPNK ? dispute.netPNK : 0) + amountShift
+          {
+            netPNK: (dispute.netPNK ? dispute.netPNK : 0) + amountShift


(dispute.netPNK || 0) + amountShift

epiqueras · 2018-03-14T22:47:03Z

src/utils/StoreProviderWrapper.js

+      )
+
+      if (_.isNull(notificationIndex)) {
+        throw new TypeError(`No notification with txHash ${txHash} exists`)


Not really a TypeError.

The linter makes me do it

We should look into making custom errors later. For cases like these and also the ones described here:
ethereum/EIPs#136

epiqueras · 2018-03-14T22:52:04Z

src/utils/StoreProviderWrapper.js

+      userProfile.notifications[notificationIndex].read = isRead
+      delete userProfile._id
+      delete userProfile.created_at
+      return new Promise(resolve => resolve(JSON.stringify(userProfile)))


unnecessary promise

epiqueras · 2018-03-14T22:52:11Z

src/utils/StoreProviderWrapper.js

+      }
+
+      userProfile.notifications[notificationIndex].read = isRead
+      delete userProfile._id


What is this for?

_id and created_at are generated on the store. You can't push a profile with an id that already exists. And it is ordered by created_at so we need to remove the timestamp so we get a new one

epiqueras · 2018-03-14T23:07:46Z

util/PromiseQueue.js

+      const returnPromise = new Promise(resolve => {
+        returnResolver = resolve
+      })
+      promise = promise.then(fn, fn).then(result => {


Nice work on this.

Just one thing. If fn throws then returnPromise will never resolve. Just add this to fix it:

promise = promise.then(fn, fn).then(res => returnResolver(res), err => returnRejecter(err))

epiqueras · 2018-03-14T23:09:05Z

tests/integration/notifications.test.js

+  let rngAddress
+  let pnkAddress
+
+  beforeAll(async () => {


You don't really need to use beforeAll. You can just make the function passed to the parent describe async and do all of this inline.

I think its still useful to have it here so if we want to add more tests to this file we don't need to pull it back out

satello · 2018-03-15T10:15:14Z

thanks for the review @epiqueras

satello added 3 commits March 14, 2018 11:49

wip

c67fb00

refactor: implement the start of a queue in store provider

9c55871

refactor: tests fixed

3c51315

satello requested review from n1c01a5 and epiqueras March 14, 2018 11:04

satello added 2 commits March 14, 2018 13:56

refactor: use different accounts for each test. bootstrap ganache wit…

b8a70f0

…h 15 accounts

chore: update ganache cli to try to fix travis error

2c0d9a5

epiqueras assigned satello Mar 14, 2018

epiqueras added the refactor label Mar 14, 2018

epiqueras added this to the March 5 -> March 19 milestone Mar 14, 2018

epiqueras suggested changes Mar 14, 2018

View reviewed changes

refactor: code review updates

c8732ef

satello merged commit 8962fea into develop Mar 15, 2018

epiqueras deleted the refactor/split_tests branch March 15, 2018 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/split tests #102

Refactor/split tests #102

satello commented Mar 14, 2018 •

edited

Loading

coveralls commented Mar 14, 2018 •

edited

Loading

epiqueras left a comment

epiqueras Mar 14, 2018

epiqueras Mar 14, 2018

epiqueras Mar 14, 2018

epiqueras Mar 14, 2018

epiqueras Mar 14, 2018

epiqueras Mar 14, 2018

satello Mar 15, 2018

epiqueras Mar 15, 2018

epiqueras Mar 14, 2018

epiqueras Mar 14, 2018

satello Mar 15, 2018

epiqueras Mar 14, 2018

epiqueras Mar 14, 2018

satello Mar 15, 2018

satello commented Mar 15, 2018

Refactor/split tests #102

Refactor/split tests #102

Conversation

satello commented Mar 14, 2018 • edited Loading

coveralls commented Mar 14, 2018 • edited Loading

Pull Request Test Coverage Report for Build 516

💛 - Coveralls

epiqueras left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satello commented Mar 15, 2018

satello commented Mar 14, 2018 •

edited

Loading

coveralls commented Mar 14, 2018 •

edited

Loading