Publish metrics test is flaky

[ghost]

Working on some HostConfiguration changes, I had the runtime_can_publish_metrics test failing https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/2871112.

[alexggh]

Looking at the logs, and it seems the reason the test failed is because the assumption that Alice produced 2 blocks is not met:

// Wait for Alice to author two blocks.
alice.wait_for_blocks(2).await;

The reason that is not happening is because a re-org is happening in the test, so instead of producing two blocks we receive two notifications for block one:

[2023-05-22 12:36:56] 2023-05-22 12:36:56.512  INFO               tokio-runtime-worker substrate: [//Alice] ✨ Imported #1 (0x46d1…3c55)    
[2023-05-22 12:36:56] 2023-05-22 12:36:56.512  INFO               tokio-runtime-worker sc_informant: [//Alice] ♻️  Reorg on #1,0x46d1…3c55 to #1,0x6866…80ca, common ancestor #0,0x7b54…4b42    
[2023-05-22 12:36:56] 2023-05-22 12:36:56.512  INFO               tokio-runtime-worker substrate: [//Alice] ✨ Imported #1 (0x6866…80ca)   

The code for wait_for_blocks is something like this, so it will count twice block one and it will exit, hence why the fail assert later down the line.

while let Some(notification) = import_notification_stream.next().await {
    if notification.is_new_best {
	    blocks.insert(notification.hash);
	    if blocks.len() == count {
		    break
	    }
    }
}

Possible solutions:

1. The quickest would be to wait more than 2 blocks before we ask for the runtime metrics, here
2. Implement a test helper alternative to wait_for_block that takes into consideration Reorgs might happen.
3. Fix wait_for_block helper in substrate, TBD if the current behaviour is correct or not.

[alexggh]

Proposed fix here: #7279

[bkchr]

3. Fix wait_for_block helper in substrate, TBD if the current behaviour is correct or not.

Good idea.

[alexggh]

3. Fix wait_for_block helper in substrate, TBD if the current behaviour is correct or not.

Good idea.

Any thoughts if the current behaviour of the the function is correct in the presence of a Reorg, just by looking at the documentation of the API, I would say it does what we expect it to do:

/// Wait for `count` blocks to be imported in the node and then exit. This function will not
/// return if no blocks are ever created, thus you should restrict the maximum amount of time of
/// the test execution.
fn wait_for_blocks(&self, count: usize) -> Pin<Box<dyn Future<Output = ()> + Send>>;

[bkchr]

I think I have written this and the idea was waiting for the chain to build n unique blocks. Not just imports.

[sandreim]

fixed in #7279

[alexggh]

I think I have written this and the idea was waiting for the chain to build n unique blocks. Not just imports.

I guess you mean something like this: paritytech/substrate#14215 ?