Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Fix flaky reputation change test #7550

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

AndreiEres
Copy link
Contributor

@AndreiEres AndreiEres commented Jul 26, 2023

Fixes #7407

That job requires more than 1ms to complete on cold cache, so reputation change triggers twice. I changed the interval significantly, same as in other test uses reputation change.

Could you please check if this works?

@AndreiEres AndreiEres requested review from koute and ggwpez July 26, 2023 16:20
@@ -471,7 +470,7 @@ fn delay_reputation_change() {
let pool = sp_core::testing::TaskExecutor::new();
let (ctx, mut handle) = make_subsystem_context::<BitfieldDistributionMessage, _>(pool);
let mut rng = dummy_rng();
let reputation_interval = Duration::from_millis(1);
let reputation_interval = Duration::from_millis(100);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let reputation_interval = Duration::from_millis(100);
let reputation_interval = Duration::from_secs(1);

Can we bump it even more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a point. It will increase the test time by a whole second. 100ms are already too much. Actual execution time, I think, is about 2ms.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't suppose we could make it so that it doesn't require any timeout at all? By, for example, waiting for something that we know must happen instead of just waiting for a predefined amount of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's not just a timeout for tests. Here we determine a value to use in the subsystem.

loop {
select! {
_ = reputation_delay => {
state.reputation.send(ctx.sender()).await;
reputation_delay = new_reputation_delay();
},
message = ctx.recv().fuse() => {
let message = match message {

@ggwpez
Copy link
Member

ggwpez commented Jul 26, 2023

Please mention the issue in your MR description so its possible to track it #7407

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
@AndreiEres AndreiEres added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 27, 2023
@ggwpez
Copy link
Member

ggwpez commented Jul 27, 2023

Test fetch_one_collation_at_a_time just failed in the CI. I assume this is unrelated?

@AndreiEres
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@AndreiEres
Copy link
Contributor Author

Test fetch_one_collation_at_a_time just failed in the CI. I assume this is unrelated?

I think so, I rebased, and the failed test is gone.

@AndreiEres AndreiEres requested a review from ggwpez August 2, 2023 13:36
@AndreiEres
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e8ee999 into master Aug 4, 2023
@paritytech-processbot paritytech-processbot bot deleted the AndreiEres/fix-reputation-interval branch August 4, 2023 14:27
s0me0ne-unkn0wn pushed a commit that referenced this pull request Aug 15, 2023
* Fix flaky reputation change test

* Remove fixme

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

---------

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
Status: done
Development

Successfully merging this pull request may close these issues.

Randomly failing test due to a race condition
4 participants