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

Tests: parse port from substrate binary output to avoid races #501

Merged
merged 4 commits into from
Apr 6, 2022

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Apr 6, 2022

Start the binary with port=0 and parse out the actual port number it starts on from its output. This (hopefully) avoids any races between finding and makign use of available ports. It also has the nice side effect of avoiding the need for any arbitrary sleeping to wait for the RPC server to start up.

@jsdw jsdw requested a review from a team April 6, 2022 12:38
.expect("failed to obtain next line from stdout for port discovery");

// does the line contain our port (we expect this specific output from substrate).
let line_end = match line.rsplit_once("Listening for new connections on 127.0.0.1:") {
Copy link
Member

Choose a reason for hiding this comment

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

this will not work once we update to jsonrpsee in substrate but a later question I guess.

Copy link
Member

Choose a reason for hiding this comment

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

DQ: Why wont this work with the update? Just out of curiousity.

Copy link
Collaborator Author

@jsdw jsdw Apr 6, 2022

Choose a reason for hiding this comment

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

Aah, different log output for the port I guess?

(I wonder what the log output will be? it should be easy enough to look for both when the time comes, to continue to support older nodes :))

(I do wish there was a better way to find the port that a thing is running on; maybe I should look into this a bit more! @lexnv any ideas?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Never tried it, but maybe you could use something like https://docs.rs/netstat/latest/netstat/ to find the open ports of the spawned process, then might be easy enough to find the websockets port?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ooh thanks for the pointer; I might have a look into this!

Copy link
Member

Choose a reason for hiding this comment

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

Aah, different log output for the port I guess?

Yeah, but we could change to have this log in jsonrpsee it's quite useful.

test-runtime/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

👍

Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
@jsdw jsdw merged commit 10627fb into master Apr 6, 2022
@jsdw jsdw deleted the jsdw-port-allocation branch April 6, 2022 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants