Unit Tests + BugFixes #15

dino-rodriguez · 2018-08-07T16:37:36Z

Thanks for the big initial push on the plugin @kincaidoneil!

jest unit tests for all functions
fixes for asym-server
- fixed proxies not persisting account changes to store
- fixed proxies not able to access plugin store
- fixed fee estimation logic returns in gwei as expected (was previously wei)
- fixed Trace class to properly catch when using 'throw Trace(..)'
- fixed fulfill/reject timer on sending prepares
- fixed formatAmount error on negative numbers
- fixed channel state to be 0 not 1 for open channel
- fixed wei <> gwei conversion missing in balance logic
- fixed timer interval to be in milliseconds (previously seconds)
- other minor stuff

Write index, file, create simple instance, create simple memory store for tests.

jsf-clabot · 2018-08-07T16:37:38Z

All committers have signed the CLA.

sharafian · 2018-08-14T23:18:34Z

src/index.ts

@@ -147,7 +152,8 @@ export default class MachinomyServerPlugin extends MiniAccountsPlugin {
        accountName,
        balance: new BigNumber(0)
      }, {
-        set (obj, prop, val) {
+        set: (obj, prop, val) => {
+          obj[prop] = val


This is redundant with the Reflect.set call. I'm also not familiar enough with Proxy/Reflect to know why this doesn't cause an infinite loop when we set inside of the set handler, but if the tests work then it must not be an issue

Will change to return true instead of the Reflect.set call -- forgot to update that. Previously, the object property was not set prior to putting the object in the store (thus storing an old object).

I'm not too familiar with proxies either, so I'm not sure why it doesn't cause an infinite loop. In the MDN example linked below, Reflect.set is used within the set handler of the proxy. Yet the interceptions section beneath the example states that Reflect.set triggers the trap as well.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/set

sharafian · 2018-08-14T23:19:51Z

src/index.ts

@@ -191,7 +197,7 @@ export default class MachinomyServerPlugin extends MiniAccountsPlugin {
    if (account.isBlocked) {
      throw new Error(`Cannot connect to blocked account ${account.accountName}`)
    }
-
+ 


Looks like there's trailing whitespace here, is lint configured for this repo?

Handled, thanks. It is -- didn't run the linter before pushing, will do so next time.

sharafian · 2018-08-14T23:20:56Z

src/index.ts

@@ -266,20 +271,21 @@ export default class MachinomyServerPlugin extends MiniAccountsPlugin {
      account.balance = newBalance
      this._log.trace(`account ${account.accountName} debited ${formatAmount(amount, 'eth')}, new balance ${formatAmount(newBalance, 'eth')}`)

+      // Expiry timer for reject
+      let timer = new Promise(resolve => setTimeout(() => {


make sure to clear this timer after the Promise.race

sharafian · 2018-08-14T23:23:44Z

src/index.ts

@@ -348,7 +354,7 @@ export default class MachinomyServerPlugin extends MiniAccountsPlugin {

        // isFunding should be toggled after all non-funding related aysnc operations have completed, but before any balance changes occur
        if (account.isFunding) {
-          throw new Trace(`Failed to pay account ${account.accountName}: another funding event was occuring simultaneously.`)
+          throw new Trace(`another funding event was occuring simultaneously.`)


I don't think an in-flight funding transaction should stop packets from being processed. Unless funding has a different meaning here than in the XRP context (adding funds to the channel) it should happen concurrently with traffic on the plugin. An in flight fund should just prevent more fund transactions from being issued until it's done.

sharafian · 2018-08-14T23:24:14Z

test/plugin.test.js

+const Store = require('./util/memStore')
+const BtpPacket = require('btp-packet')
+const IlpPacket = require('ilp-packet')
+const debug = require('debug')('ilp-plugin-eohereum-asym-server:test')


eohereum -> ethereum

sharafian · 2018-08-14T23:25:31Z

test/plugin.test.js

+
+/* TODO Mock for web3, HDWallet, and Machinomy API. */
+async function createPlugin() {
+  /* Silence console log output from provider creation. */


Can you make an upstream PR to use debug instead of console.log? It's quite strange for a library to use console.log without an option to turn it off

sharafian · 2018-08-14T23:26:47Z

test/plugin.test.js

+
+beforeEach(async () => {
+  try {
+    this.plugin = await createPlugin() 


I'm not 100% sure how this works; this shouldn't be bound to anything in this context because it's an arrow function. I guess it's using the this of the global scope? Either way, functions in the tests should be changed to function instead of arrow function syntax.

The reason I did this is because Jest does not share the 'this' context across beforeEach, afterEach, and the test itself (as mocha does).

jestjs/jest#3553

To get the shared variable access, I either had to use arrow functions with 'this', or define variables as globals (as indicated above). I chose to use 'this' because I thought it would be visually more clear which variables were acting as globals. Can change it to the latter if it you think it is still best to have each test function individually scoped.

Ah, I see. I guess I'm just not as familiar with jest. If this is the common way that it's done then that's fine

sharafian · 2018-08-14T23:27:32Z

test/plugin.test.js

+          expect(fee.toNumber()).toBeLessThan(1200000) 
+        }))
+      } catch (e) {
+        throw e


catch(e) { throw e } seems like a no-op

sharafian · 2018-08-14T23:29:14Z

test/plugin.test.js

+      this.mockAccount.isBlocked = true
+      try {
+        await this.plugin._connect(this.mockAccountAddress, {})
+        throw Error('Test Error: should trigger catch')


Instead of doing this, you can do expect(this.plugin._connect(this.mockAccountAddress, {})).to.be.rejectedWith('...')

sharafian · 2018-08-14T23:31:55Z

test/plugin.test.js

+          jest.runOnlyPendingTimers()
+          await protocolDataResponse
+          expect(oldBalance).toEqual(this.mockAccount.balance - this.mockIlpPrepare.amount)
+        } catch (e) {}


Why are errors being swallowed? that means this test isn't testing anything

The intention here (which I did in a couple of other places) was that for longer functions (like handlePrepareResponse), I wanted to test the status of something like a specific like an account's balance. Since it changed so many times over the course of the function, I wanted to capture it during a specific phase -- thus I caught errors which stopped the function early to test the status of the account balance at that point. Do you think it's best to forgoe these tests? I know unit testing logic should not peep inside functions too deeply, but I found this useful in finding a few bugs.

Add jest unit tests for main plugin. WIP -- still have not finished. Create simple memstore for tests. Jest config in package.json to work properly with node (had a bug when not explicitly specifying node environment).

Fee estimation: now returns in gwei instead of wei as intended. Account store: assign anonymous function to proxy to fix `this` context (was not setting to plugin store). Also, set the object property value on the set trap (was not persisting account changes to store). Still a WIP, various TODOs throughout.

Throwing traces fixed (previously not catching). Replace ternary operator logic of _handlePrepareResponse which was not working before. Promise race in _handleCustomData was outputting invalid structure for ILP reject, now properly returns.

* formatAmount fixed to handle negative numbers * channel state should be of 0 not 1 for open channel * added wei <> gwei conversion missing in balance logic * timer adjusted from seconds to milliseconds

kincaidoneil · 2018-08-21T15:17:07Z

Circle on Node 10 is failing when it tries to build sqlite3, a Machinomy dependency--but merging this anyways since we're in the process of removing that.

dino rodriguez added 2 commits August 2, 2018 10:39

Begin eth asym server tests

6bf24d8

Write index, file, create simple instance, create simple memory store for tests.

test skeleton

d130d42

dino-rodriguez requested review from kincaidoneil, 0xASK, sharafian and emschwartz August 7, 2018 16:37

sharafian requested changes Aug 14, 2018

View reviewed changes

Austin King and others added 11 commits August 17, 2018 13:54

currency scale check

b74ea59

create plugin base done

b278412

Default framework and packages installed

fc3f507

test: unit tests

c0ea3be

Add jest unit tests for main plugin. WIP -- still have not finished. Create simple memstore for tests. Jest config in package.json to work properly with node (had a bug when not explicitly specifying node environment).

test: more unit tests

5c7b002

fix: trace class + timer race functioning

7fcd0ee

Throwing traces fixed (previously not catching). Replace ternary operator logic of _handlePrepareResponse which was not working before. Promise race in _handleCustomData was outputting invalid structure for ILP reject, now properly returns.

chore: eslint and npm scripts

ba7377a

fix: assorted bug fixes

dac84c8

* formatAmount fixed to handle negative numbers * channel state should be of 0 not 1 for open channel * added wei <> gwei conversion missing in balance logic * timer adjusted from seconds to milliseconds

test: all functions unit tested

e0a7ba5

chore: install & configure eslint

766b0e4

dino-rodriguez force-pushed the dr-unit-tests branch from 8795b9a to 766b0e4 Compare August 17, 2018 20:55

Update CircleCI config to run tests

2cac0d8

kincaidoneil merged commit ce70946 into ko-refactor Aug 21, 2018

kincaidoneil deleted the dr-unit-tests branch September 20, 2018 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit Tests + BugFixes #15

Unit Tests + BugFixes #15

dino-rodriguez commented Aug 7, 2018 •

edited

Loading

jsf-clabot commented Aug 7, 2018 •

edited

Loading

sharafian Aug 14, 2018

dino-rodriguez Aug 15, 2018

sharafian Aug 14, 2018

dino-rodriguez Aug 15, 2018

sharafian Aug 14, 2018

sharafian Aug 14, 2018

sharafian Aug 14, 2018

sharafian Aug 14, 2018

dino-rodriguez Aug 15, 2018

sharafian Aug 14, 2018

dino-rodriguez Aug 15, 2018 •

edited

Loading

sharafian Aug 15, 2018

sharafian Aug 14, 2018

sharafian Aug 14, 2018

sharafian Aug 14, 2018

dino-rodriguez Aug 15, 2018

kincaidoneil commented Aug 21, 2018

Unit Tests + BugFixes #15

Unit Tests + BugFixes #15

Conversation

dino-rodriguez commented Aug 7, 2018 • edited Loading

jsf-clabot commented Aug 7, 2018 • 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

dino-rodriguez Aug 15, 2018 • 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

kincaidoneil commented Aug 21, 2018

dino-rodriguez commented Aug 7, 2018 •

edited

Loading

jsf-clabot commented Aug 7, 2018 •

edited

Loading

dino-rodriguez Aug 15, 2018 •

edited

Loading