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

Issue #575 split test_lib.js #613

Merged
merged 12 commits into from Jan 10, 2019

Conversation

Projects
None yet
4 participants
@elpiel
Copy link
Contributor

elpiel commented Jan 3, 2019

Hello,
I already separated the ComitConf and the LQS, still things to be changed there, just want to check if I am on the right direction.

Resolves #575

  • Balance util - moved the functions for balances of eth and btc + logging
  • Bitcoin Rpc Client Config
  • Bitcoin Wallet
  • Comit Node Config
  • Ethereum wallet
  • Ledger Query Service Config
  • Wallet Config - containing both wallets
  • Web3 Config

PS: I also see a lot of requires or constants that are not used, is this intended? I will not change the signatures of the methods, but should I clean up some unused requires and variables/constants?


  • RTM (Ready To Merge)

@mergify mergify bot added the work-in-progress label Jan 3, 2019

@D4nte

This comment has been minimized.

Copy link
Member

D4nte commented Jan 3, 2019

@elpiel yes, if the requires are not needed please remove them. They may be due to copy-pasta.

Also, please do feedback on your development experience and let us know if we should update documentation or tooling to make the experience smoother and this project easier to contribute to.

Show resolved Hide resolved api_tests/comit_node_conf.js Outdated
@D4nte
Copy link
Member

D4nte left a comment

👍 so far so good. Minor comments on naming.

Show resolved Hide resolved api_tests/ledger_query_service_conf.js Outdated
@elpiel

This comment has been minimized.

Copy link
Contributor Author

elpiel commented Jan 4, 2019

@elpiel yes, if the requires are not needed please remove them. They may be due to copy-pasta.

Also, please do feedback on your development experience and let us know if we should update documentation or tooling to make the experience smoother and this project easier to contribute to.

So in terms of development experience, indeed I think some stuff can be added to the Readme. I was pretty new to the prettier code formatter ( pun intended 😹 ). For example: Adding the separate test tasks, but also maybe having the option (because at least I didn't spend a lot of time trying to find if there is one) to run only the JS test. Or having the prettier task for formatting the JS as well in the Readme - "This is how you run test: 1) JS .. 2) LQS only tests ... " and etc.

For now that's it I think, if there is something else I will update you.

I've also update the PR with list of things that need to be done, it's not full yet though but also I want your feedback on it.

@elpiel elpiel force-pushed the elpiel:issue-575-split-test_lib_js branch from 4610a81 to 732a267 Jan 4, 2019

@elpiel elpiel force-pushed the elpiel:issue-575-split-test_lib_js branch from e2c96f5 to 7228043 Jan 5, 2019

@elpiel

This comment has been minimized.

Copy link
Contributor Author

elpiel commented Jan 5, 2019

I think that's much better now. There are definitely things that can be improved more, but since it's only about splitting I decided to leave it like this. If you have anything else you'll like me to change, I will be happy to do so.

@elpiel elpiel changed the title [WIP] Issue #575 split test_lib.js Issue #575 split test_lib.js Jan 6, 2019

@D4nte D4nte requested a review from luckysori Jan 6, 2019

@D4nte D4nte self-assigned this Jan 7, 2019

@D4nte D4nte referenced this pull request Jan 7, 2019

Merged

Adds commonly used `cargo make` tasks #621

1 of 1 task complete

@D4nte D4nte requested a review from comit-network/rust-devs Jan 7, 2019

@D4nte
Copy link
Member

D4nte left a comment

Looks good to me, I'd like @LLFourn to review too.

await bitcoin_rpc_client_conf.btc_import_address(bob_final_address); // Watch only import
await bitcoin_rpc_client_conf.btc_import_address(
alice.wallet.btc().identity().address
); // Watch only import

This comment has been minimized.

@D4nte

D4nte Jan 7, 2019

Member

I know it's due to the formatting but could you move the comment to it's own line above the import?

This comment has been minimized.

@elpiel

elpiel Jan 7, 2019

Author Contributor

Yes, no, I didn't go into much details while moving code. I will do it today

@D4nte D4nte requested a review from comit-network/rust-devs Jan 7, 2019

@D4nte D4nte dismissed their stale review Jan 7, 2019

Happy with the changes. Would like @LLFourn to review

@D4nte D4nte added review and removed work-in-progress labels Jan 7, 2019

@LLFourn
Copy link
Contributor

LLFourn left a comment

Nice work. Thanks. test_lib.js was quite a mess. There's two things I'd like before we merge:

  1. Put the js files into one directory Since you've split up things into files let's put them in their own directory api_tests/lib
  2. Arrange the files by currency Let's have bitcoin.js, ethereum.js. Inside each we can have the related functions and wallets. e.g. we can delete balance_util.js and just move the bitcoin functionality to bitcoin.js and the ethereum functionality to ethereum.js. Should move bitcoin_rpc_client_conf.js to into bitcoin.js too etc.

Actually, I think we only need these files: harness.js, bitcoin.js, ethereum.js, actor.js (if you feel like renaming ComitNodeConf in the way I suggested) and lqs.js (for ledger query service).

return this._btc_wallet;
}

async send_raw_tx(hex) {

This comment has been minimized.

@LLFourn

LLFourn Jan 8, 2019

Contributor

This doesn't belong here.

This comment has been minimized.

@elpiel

elpiel Jan 8, 2019

Author Contributor

I guess we can directly put it on the Actor?

This comment has been minimized.

@LLFourn

LLFourn Jan 8, 2019

Contributor

It's a bitcoin thing so just put it in bitcoin.js. Alternatively, just delete it and call the thing on bitcoin_rpc_client directly. I don't think this function adds any value.

);
}

eth() {

This comment has been minimized.

@LLFourn

LLFourn Jan 8, 2019

Contributor

🔧 Instead of having this I think we can just put these methods on ComitNodeConf. We can then delete this file.

🔧 It would be greet to name ComitNodeConf to something else like Actor (it no longer represents the configuration of a comit node exclusively).

This comment has been minimized.

@elpiel

elpiel Jan 8, 2019

Author Contributor

Make sense! I will do it. I wasn't sure exactly what names would be good and if I had to merge things.
I'm also not sure what to do with those two places that directly use the Wallet:

const toby_wallet = wallet_conf.create();

const wallet = wallet_conf.create();

Any suggestions about that?

This comment has been minimized.

@LLFourn

LLFourn Jan 8, 2019

Contributor

@elpiel Whoops. You're right. Forget about removing wallet for now then (probably call it wallet instead of wallet_conf).

const fs = require("fs");

class ComitNodeConf {
constructor(name, bitcoin_utxo) {

This comment has been minimized.

@LLFourn

LLFourn Jan 8, 2019

Contributor

This bitcoin_utxo is never used. Let's delete it.

This comment has been minimized.

@elpiel

elpiel Jan 8, 2019

Author Contributor

Wasn't sure if it's intended for future use.

@elpiel

This comment has been minimized.

Copy link
Contributor Author

elpiel commented Jan 8, 2019

@LLFourn Can you tell me where should the test_lib.js functions left be living in? We can put them in harness.js, although there are quite some stuff there already

@D4nte D4nte removed their assignment Jan 8, 2019

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 9, 2019

@elpiel Good question. I don't think we should put util stuff in harness because it's an executable file rather than a library. There are two functions in there that are just returning global variables -- I'm not sure why we did that. For test_rng and sleep maybe just put them in a util.js if you can't find a better place.

Obviously the mint erc20 tokens thing should just go in ethereum.js (and should be a function on the wallet where the owner of the wallet is the owner of the contract if you wanted to do some more improvements).

@elpiel

This comment has been minimized.

Copy link
Contributor Author

elpiel commented Jan 9, 2019

@LLFourn Now that I moved stuff around, I have an issue with the tests. Also can you give any advice for the web3_conf?
It has something to do with the moving stuff around, but I honestly don't know why it is a problem since the util.js is loaded in harness.js

PS: I am just getting started with blockchain and cryptocurrencies and wasn't sure about the erc20 tokes

    const logger = global.harness.logger;
                                  ^

TypeError: Cannot read property 'logger' of undefined
    at Object.<anonymous> (/media/elpiel/First/PROJECTS/rust-lang/comit-rs/api_tests/lib/util.js:17:35)
    at Module._compile (internal/modules/cjs/loader.js:721:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:732:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:657:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/media/elpiel/First/PROJECTS/rust-lang/comit-rs/api_tests/lib/web3_conf.js:1:63)
    at Module._compile (internal/modules/cjs/loader.js:721:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:732:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:657:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/media/elpiel/First/PROJECTS/rust-lang/comit-rs/api_tests/lib/bitcoin.js:3:19)
    at Module._compile (internal/modules/cjs/loader.js:721:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:732:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:657:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/media/elpiel/First/PROJECTS/rust-lang/comit-rs/api_tests/harness.js:1:79)
    at Module._compile (internal/modules/cjs/loader.js:721:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:732:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:657:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at /media/elpiel/First/PROJECTS/rust-lang/comit-rs/api_tests/node_modules/mocha/lib/mocha.js:250:27
    at Array.forEach (<anonymous>)
    at Mocha.loadFiles (/media/elpiel/First/PROJECTS/rust-lang/comit-rs/api_tests/node_modules/mocha/lib/mocha.js:247:14)
    at Mocha.run (/media/elpiel/First/PROJECTS/rust-lang/comit-rs/api_tests/node_modules/mocha/lib/mocha.js:576:10)
    at Object.<anonymous> (/media/elpiel/First/PROJECTS/rust-lang/comit-rs/api_tests/node_modules/mocha/bin/_mocha:637:18)
    at Module._compile (internal/modules/cjs/loader.js:721:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:732:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:774:12)
    at executeUserCode (internal/bootstrap/node.js:342:17)
    at startExecution (internal/bootstrap/node.js:276:5)
    at startup (internal/bootstrap/node.js:227:5)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)```

@elpiel elpiel force-pushed the elpiel:issue-575-split-test_lib_js branch from 35025b2 to 8287709 Jan 9, 2019

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 9, 2019

@elpiel No worries. Thanks! Since you've done most of the heavy lifting, I think it's faster if I just fix the logging thing and cleanup any remaining things. I'll make a PR to your fork and then we can merge this stuff :)

Clean up aftermath of splitting test_lib.js
- Put erc20 and web3 stuff in ethereum
- Use BigInt instead of Web3.util.BN
- Remove most of the stuff from `util.js`
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jan 10, 2019

CLA assistant check
All committers have signed the CLA.

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 10, 2019

@elpiel Looks like I was able to push to your branch with 85d9c3e. This looks good to me now :).

Don't worry these terms can be very confusing. web3? erc20? If the name isn't straightforward it's probably ethereum related!

@elpiel

This comment has been minimized.

Copy link
Contributor Author

elpiel commented Jan 10, 2019

@LLFourn thanks a lot! The changes do make sense. I hope to contribute to the Rust part soon as well.

@LLFourn LLFourn merged commit 692aeb1 into comit-network:master Jan 10, 2019

1 check passed

license/cla Contributor License Agreement is signed.
Details

@wafflebot wafflebot bot removed the review label Jan 10, 2019

@LLFourn

This comment has been minimized.

Copy link
Contributor

LLFourn commented Jan 10, 2019

@elpiel No worries. Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment