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

test: xud-simulation migration #852

Merged
merged 34 commits into from
Apr 6, 2019
Merged

test: xud-simulation migration #852

merged 34 commits into from
Apr 6, 2019

Conversation

moshababo
Copy link
Collaborator

Closes #765.

This PR is migrating xud-simulation under xud/test/simulation.

I changed .travis to have language: go instead of language: node_js. Otherwise go1.7.4 is being shipped, which isn't sufficient for Go Modules.

The new .travis file is a bit messy now, combining 2 environments. Feel free to suggest improvements.

Also, please execute it on your local machine by running npm run test:sim (go1.11.x installation is required).

@moshababo moshababo requested review from a user and sangaman March 23, 2019 18:23
@ghost ghost assigned moshababo Mar 23, 2019
@ghost ghost added the in progress label Mar 23, 2019
@moshababo moshababo requested a review from reliveyy March 24, 2019 04:21
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Successfully ran the test suites locally - good job 👍 . Didn't manage to go over the code in detail, but in general looks very nice - excited to have these run on every build and prevent bugs in the core functionality :).

test/simulation/README.md Outdated Show resolved Hide resolved
GO111MODULE=on go test -v
````

## Network Scenarios Tests
Copy link

Choose a reason for hiding this comment

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

Really like how much initial coverage is included.

@kilrau
Copy link
Contributor

kilrau commented Mar 25, 2019

You mentioned after merging this there will be some fixes necessary to xud/test/simulation. Can this be done before merging? @moshababo

@moshababo
Copy link
Collaborator Author

@kilrau i'm not sure which fixes are you referring to.
I left some TODOs in the code (mostly for better code maintenance), and we need to add more coverage, but besides that, this should be merged soon so any new PR which breaks the protocol can be detected.

@sangaman
Copy link
Collaborator

sangaman commented Mar 26, 2019

Would it be an option to manually install the specific go version we want via the travis script before runnings tests, instead of switching the language to go? My concern is not being able to test multiple version of nodejs or specify which ones to use, right now it looks like it's using version 8 only which is no longer "active." I guess it would make the test runtime longer though?

Edit: I do see that we are installing the stable version of node manually which I suppose is fine, but we do still lose the ability to test multiple node versions which seems more important than testing multiple go versions.

Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

Fantastic work. In trying to run this locally, I'm getting this error:

=== RUN   TestExchangeUnionDaemon
2019/03/26 09:29:33 installing dependencies...
--- FAIL: TestExchangeUnionDaemon (9.53s)
    xud_test.go:155: Error outside of test: *errors.errorString installation error: starting lnd clone...
        finished lnd clone
        starting lnd make...
         Compiling dependencies.
        dep ensure -v
        Makefile:116: recipe for target 'dep' failed
        unable to make lnd
        
        
        /home/mcnallydp_gmail_com/xud/test/simulation/xud_test.go:191 (0x9ea82c)
                TestExchangeUnionDaemon: ht.Fatalf("%v", err)
        /usr/local/go/src/testing/testing.go:827 (0x4f799f)
                tRunner: fn(t)
        /usr/local/go/src/runtime/asm_amd64.s:1333 (0x45d131)
                goexit: BYTE    $0x90   // NOP
FAIL

I've tried manually running go get -u github.com/golang/dep/cmd/dep but that doesn't seem to help. Any ideas?

@@ -0,0 +1,579 @@
// Copyright 2018 The Exchange Union Developers
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this and the other .proto files, would it be possible to reference the existing files in the repository (outside the simulation folder)? My concern is that we'd have to manually change files in two places and that they could fall out of sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I wanted to do that after the migration and forgot. the gen_protos.sh will still need to be triggered after changes though.

@@ -31,6 +31,7 @@
"test:p2p": "mocha -r ts-node/register test/p2p/*",
"test:p2p:watch": "mocha -r ts-node/register test/p2p/* --watch --watch-extensions ts",
"test:crypto": "mocha -r ts-node/register test/crypto/*",
"test:sim": "(cd test/simulation && GO111MODULE=on go test -v)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this won't work on Windows, something that may be worth doing here is using a cross-os script that prints something like "simulation tests not supported on windows" for windows and uses this script for linux/mac.

test/simulation/LICENSE Outdated Show resolved Hide resolved
Installation & Usage:

```bash
$ GO111MODULE=on go test -v
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to change this to npm run test:sim?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't do it so it will be treated as independent folder to be run directly. But since we want to create a dependency with the .proto files, I can that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I fixed that, but didn't push yet because I wanted you to see the current travis failure status.

@kilrau
Copy link
Contributor

kilrau commented Mar 26, 2019

@kilrau i'm not sure which fixes are you referring to.
I left some TODOs in the code (mostly for better code maintenance), and we need to add more coverage, but besides that, this should be merged soon so any new PR which breaks the protocol can be detected.

Misunderstanding. Just ignore what I said.

@sangaman
Copy link
Collaborator

FYI, I got past my error above (I think I needed to add to the go/bin folder to my path) but now I'm hitting this error when trying to run npm run test:sim

=== RUN   TestExchangeUnionDaemon
2019/03/28 02:12:31 installing dependencies...
2019/03/28 02:13:49 
starting lnd clone...
finished lnd clone
starting lnd make...
 Compiling dependencies.
dep ensure -v
 Building debug lnd and lncli.
go build -v -tags="dev" -o lnd-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.4.2-beta-1255-ga62cd86a8bb9a993cc2ad14c8fcae20bc2e2f967" github.com/lightningnetwork/lnd
go build -v -tags="dev" -o lncli-debug -ldflags "-X github.com/lightningnetwork/lnd/build.Commit=v0.4.2-beta-1255-ga62cd86a8bb9a993cc2ad14c8fcae20bc2e2f967" github.com/lightningnetwork/lnd/cmd/lncli
finished lnd make
--- FAIL: TestExchangeUnionDaemon (154.73s)
    xud_test.go:232: ltcd: launching node...
    xud_test.go:244: ltcd: 200 blocks generated
    xud_test.go:270: lnd-ltc: launching network...
    xud_test.go:305: btcd: launching node...
    xud_test.go:320: btcd: 200 blocks generated
    xud_test.go:352: lnd-btc: launching network...
   xud_test.go:371: launching xud network...
    xud_test.go:155: Error outside of test: *errors.errorString cannot set up xud network: timeout waiting for TLS cert file to be created after 30 seconds
        /home/mcnallydp_gmail_com/xud/test/simulation/xud_test.go:373 (0x9eb9c8)
                TestExchangeUnionDaemon: ht.Fatalf("cannot set up xud network: %v", err)
        /usr/local/go/src/testing/testing.go:827 (0x4f799f)
                tRunner: fn(t)
        /usr/local/go/src/runtime/asm_amd64.s:1333 (0x45d131)
                goexit: BYTE    $0x90   // NOP
    xud_test.go:155: Error outside of test: *errors.errorString cannot tear down xud network harness: failed to kill process: os: process already finished
        /home/mcnallydp_gmail_com/xud/test/simulation/xud_test.go:365 (0x9eda09)
                TestExchangeUnionDaemon.func9: ht.Fatalf("cannot tear down xud network harness: %v", err)
        /usr/local/go/src/runtime/asm_amd64.s:522 (0x45b41b)
                call32: CALLFN(·call32, 32)
        /usr/local/go/src/runtime/panic.go:397 (0x42d23f)
                Goexit: reflectcall(nil, unsafe.Pointer(d.fn), deferArgs(d), uint32(d.siz), uint32(d.siz))
        /usr/local/go/src/testing/testing.go:590 (0x4f6a99)
                (*common).FailNow: runtime.Goexit()
        /usr/local/go/src/testing/testing.go:634 (0x4f6f93)
                (*common).Fatalf: c.FailNow()
        /home/mcnallydp_gmail_com/xud/test/simulation/xud_test.go:155 (0x9ea49c)
                (*harnessTest).Fatalf: h.t.Fatalf("Error outside of test: %v", stacktrace)
        /home/mcnallydp_gmail_com/xud/test/simulation/xud_test.go:373 (0x9eb9c8)
                TestExchangeUnionDaemon: ht.Fatalf("cannot set up xud network: %v", err)
        /usr/local/go/src/testing/testing.go:827 (0x4f799f)
                tRunner: fn(t)
        /usr/local/go/src/runtime/asm_amd64.s:1333 (0x45d131)
                goexit: BYTE    $0x90   // NOP
    xud_test.go:332: lnd-btc: network harness teared down
    xud_test.go:301: btcd: harness teared down
    xud_test.go:254: lnd-ltc: network harness teared down
    xud_test.go:228: ltcd: harness teared down
FAIL
exit status 1
FAIL    github.com/ExchangeUnion/xud-simulation 154.791s

@moshababo
Copy link
Collaborator Author

@sangaman you’re on Windows, right? if so, then there’s more work to do to make it compatible.

I think I needed to add to the go/bin folder to my path

Probably related to filesystem permissions. Feel free to edit install.sh to support this. Otherwise we can just wait for LND update, which will might solve this, because dep won’t be needed anymore (since it’s using Go Modules).

but now I'm hitting this error when trying to run npm run test:sim

TLS cert file isn't created. This is probably because xud process crash immediately after launch, due to failure in creating the datadir, port collision, etc. To check this, you can disable the xud cleanup (Config.go, XudCleanup), and then look at the logs.

If you're not on Windows, let me know.

@kilrau
Copy link
Contributor

kilrau commented Apr 4, 2019

Can "not finding any routes" actually be a travis problem? Any ideas what else it could be? @moshababo @erkarl

@ghost
Copy link

ghost commented Apr 4, 2019

@sangaman this is a longshot, but could it be a firewall rule blocking the execution? I ran the tests again without problems on a fresh local environment. I did, however, have to allow some new firewall rules.

@moshababo
Copy link
Collaborator Author

@sangaman in regards to your cloud instance run failure, it looks like a filesystem error, wheres lnd is failing to create its macaroon file. In this case, you won't see any of the log folders, because these are xud logs, which didn't even start. lnd's logs are not being written to files, but I can add a support for that.

Will try to investigate the "NoRouteFound" problem some more. Also, will see if manually installing go on travis won't cause issues.

@moshababo
Copy link
Collaborator Author

Good news - I found and fixed a problem which caused the NoRouteFound error.
@sangaman please review my latest commit ("fix maker getRoutes bug") before approving this PR, because it changes non-test xud code.

@moshababo
Copy link
Collaborator Author

Travis builds were fixed after moving back to language: node_js.
Sometimes one of the 2 builds is failing due to weird grpc error, but after restarting the build, it seems to always pass.

So the PR looks ready to me. @sangaman please try run it again on your environment, and if you still have issues, let's discuss and try to debug. I don't think it should block from merging.

Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

Awesome, again this is great work.

Do you know why the change to Swaps.ts was necessary? It looks to me like the code does the same thing (although I think it's better organized this way anyhow, since the block checking for the existence of routes didn't need to be in the try block).

@sangaman
Copy link
Collaborator

sangaman commented Apr 5, 2019

I agree this is ready to merge (as we've said we can deal with non-showstopper issues down the road), just needs to be squashed into 1 or 2 commits. If you can do that today @moshababo I'd say go for it, otherwise I will in the evening my time.

@moshababo
Copy link
Collaborator Author

@sangaman I did change the code structure a bit, but the needed change was to check lnd routes to the taker lnd pubkey, where previously it was:

deal.makerToTakerRoutes = await swapClient.getRoutes(takerAmount, deal.peerPubKey);

having deal.peerPubKey = peer.nodePubKey. This was probably a leftover from previous refactoring.

Thanks for the feedbacks, i'm merging.

@moshababo moshababo merged commit f5f1876 into master Apr 6, 2019
@ghost ghost removed the in progress label Apr 6, 2019
@moshababo moshababo deleted the sim-travis branch April 6, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants