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

ignore key_server_cluster randomly failing tests #9639

Merged
merged 3 commits into from
Sep 25, 2018

Conversation

debris
Copy link
Collaborator

@debris debris commented Sep 24, 2018

related to #9635

@debris debris added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M4-core ⛓ Core client code / Rust. labels Sep 24, 2018
@5chdn 5chdn added this to the 2.2 milestone Sep 25, 2018
@debris debris requested a review from svyatonik September 25, 2018 09:52
@svyatonik svyatonik added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 25, 2018
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm, would be nice to have an explanation comment though

@@ -1432,6 +1432,7 @@ pub mod tests {
}

#[test]
#[ignore]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like to see a comment explaining why a test ignored, this helps later on checking on this case and also understanding the reasoning behind this.
I think the link to #9635 would be good here.

Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

LGTM as a temporary solution. Two comments:

  1. Those failings are likely just caused by slow CI. One way to re-enable those tests in the short term is just to change TIMEOUT to be a higher value (rather than 300ms).
  2. For the long term, we should consider changing the TIMEOUT to use exponential back-off (same strategy used by many other async tests, such as in gnu toolchain).

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM. Similar to what @sorpaas suggested when writing tests that rely on timeouts we could have a dillation factor, all timeouts would be multiplied by this factor. This way we could have a global parameter to tune the tests to run properly on slower machines (it's still flaky though).

@debris debris merged commit 7f9a9e2 into master Sep 25, 2018
@debris debris deleted the key-server-cluster-ignore branch September 25, 2018 13:15
dvdplm added a commit that referenced this pull request Sep 27, 2018
…mon-deps

* origin/master:
  ethereum libfuzzer integration small change (#9547)
  cli: remove reference to --no-ui in --unlock flag help (#9616)
  remove master from releasable branches (#9655)
  ethcore/VerificationQueue don't spawn up extra `worker-threads` when explictly specified not to (#9620)
  RPC: parity_getBlockReceipts (#9527)
  Remove unused dependencies (#9589)
  ignore key_server_cluster randomly failing tests (#9639)
  ethcore: handle vm exception when estimating gas (#9615)
  fix bad-block reporting no reason (#9638)
  Use static call and apparent value transfer for block reward contract code (#9603)
  HF in POA Sokol (2018-09-19) (#9607)
  bump smallvec to 0.6 in ethcore-light, ethstore and whisper (#9588)
  Add constantinople conf to EvmTestClient. (#9570)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants