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

fix: Find substrate port on different log lines #536

Merged
merged 2 commits into from
May 12, 2022
Merged

fix: Find substrate port on different log lines #536

merged 2 commits into from
May 12, 2022

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented May 12, 2022

Updating substrate node to the latest version would result in an infinite loop for our integration testing.

The root cause of this issue is that the substrate switched to json-rpsee.
This included changing the log message we are looking for when finding the port of the substrate.

Ensure that the port can be obtained from both new and old versions of the substrate node.

Testing Done

Before

test client::chain_subscribe_blocks has been running for over 60 seconds
test client::chain_subscribe_finalized_blocks has been running for over 60 seconds
test client::fetch_block has been running for over 60 seconds

After

test client::chain_subscribe_finalized_blocks ... ok
test client::fetch_block ... ok
test client::chain_subscribe_blocks ... ok
test client::fetch_read_proof ... ok

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
.rsplit_once("Listening for new connections on 127.0.0.1:")
{
None => {
match line.rsplit_once("Running JSON-RPC WS server: addr=127.0.0.1:")
Copy link
Member

@niklasad1 niklasad1 May 12, 2022

Choose a reason for hiding this comment

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

you could also do: https://github.com/paritytech/substrate/blob/master/bin/node/cli/tests/common.rs#L164-#L167

then just parse it as SocketAddr then call port on it.

would be better than to assume that the address is 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.

sorry, I didn't think about making the log easy to parse :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the testing pov I guess it's ok assuming 127.0.0.1 though (ie you could start it on 0.0.0.0, but you couldn't then connect to 0.0.0.0)?

Copy link
Member

Choose a reason for hiding this comment

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

it's fair assumption, I was thinking that some weird platforms could actually have some other loopback address than 127.0.0.1. Picking hair here just ignore me but I was more thinking about having to avoid parsing the port manually :P

I think you can connect to 0.0.0.0 but it means all addresses IIRC.

@@ -168,22 +168,30 @@ fn find_substrate_port_from_output(r: impl Read + Send + 'static) -> u16 {
BufReader::new(r)
.lines()
.find_map(|line| {
let line = line
.expect("failed to obtain next line from stdout for port discovery");
let line =
Copy link
Collaborator

@jsdw jsdw May 12, 2022

Choose a reason for hiding this comment

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

I think you could simplify this matching code to be:

// does the line contain our port (we expect this specific output from substrate).
let line_end = line
    .rsplit_once("Listening for new connections on 127.0.0.1:")
    .or_else(|| line.rsplit_once("Running JSON-RPC WS server: addr=127.0.0.1:"))
    .map(|(_,port_str)| port_str)?;

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Other than the suggestion LGTM

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv merged commit ae59c8a into master May 12, 2022
@lexnv lexnv deleted the fix_ci branch May 12, 2022 14:29
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.

3 participants