Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

fix(cli): make swarm addrs more resilient #2083

Merged
merged 4 commits into from
May 22, 2019
Merged

Conversation

jacobheun
Copy link
Contributor

Dont fail if decapsulation of ipfs fails. Go node addresses typically wont contain their peerid

Fixes #1575

Dont fail if decapsulation of ipfs fails. Go node addresses typically wont contain their peerid
@jacobheun jacobheun requested a review from alanshaw May 21, 2019 15:06
@alanshaw
Copy link
Member

Nice! Please can we have a test?

@jacobheun
Copy link
Contributor Author

@alanshaw sure, do you have a recommendation for testing the cli, it's been a while. I don't believe I can change addresses on the node via the api, so the options I can think of would be to add a go node to the test suite (this feels like it should go an interop though), or just mock and test the cli handler directly.

@alanshaw
Copy link
Member

just mock and test the cli handler directly.

This please 😁

@jacobheun
Copy link
Contributor Author

I added a test for the cli handler. Since it's verifying process.stdout, the test cleanup isn't as clean as I'd like. If we just use the afterEach hook we'll lost the mocha output. We could stub the print function, but that would require a change to how it's being used in the cli file in order for the stub to work.

If we planned on adding more cli tests that just target the handlers directly (which would make cli tests much faster) we could build out some utilities to make the individual tests easier/cleaner to write.

@alanshaw
Copy link
Member

@jacobheun if you change the handler to build up a string for the output and then return it it'll get printed automatically. It might save you from having to stub process.stdout and allows you to more easily verify the output.

In the future we might want to think about passing print in via argv for easier testing.

@jacobheun
Copy link
Contributor Author

if you change the handler to build up a string for the output and then return it it'll get printed automatically.

Ah, perfect! I updated the tests to check the returned output. I think testing the output returned from the handler is great. Then we only need to verify that bin is calling print appropriately.

Here is some truncated output running locally against an 0.36 rc daemon:

$ node src/cli/bin.js swarm addrs
QmSoLnSGccFuZQJzRadHn95W2CrSFmZuTdDWP8HXaHca9z (11)
        /ip4/104.236.176.52/tcp/4001
        /ip6/2604:a880:1:20::1f9:9001/tcp/4001
        /ip6/::1/tcp/4001
        /ip6/2604:a880:1:20::1f9:9001/tcp/4001
        /ip6/fce3:c53b:c3c5:2f54:8bb0:b6d9:898e:f140/tcp/4001
        /ip4/127.0.0.1/tcp/8081/ws
        /ip4/127.0.0.1/tcp/4001
        /ip4/104.236.176.52/tcp/4001
        /ip4/10.12.0.8/tcp/4001
        /ip4/172.17.0.1/tcp/4001
        /p2p-circuit
QmaCpDMGvV2BGHeYERUEnRQAwe3N8SzbUtfsmvsqQLuvuJ (4)
        /ip4/104.131.131.82/tcp/4001
        /p2p-circuit
        /ip4/127.0.0.1/tcp/4001
        /ip4/104.131.131.82/tcp/4001

@alanshaw
Copy link
Member

Yeah it works for small non-streaming APIs, but print is still needed for commands like ping.

@alanshaw alanshaw merged commit 3792b68 into master May 22, 2019
@alanshaw alanshaw deleted the fix/swarm-addrs-cli branch May 22, 2019 11:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jsipfs swarm addrs errors
2 participants