Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Shadow Ban unit tests are flaky due to random sleep times #8150

Closed
anoadragon453 opened this issue Aug 24, 2020 · 3 comments · Fixed by #8152
Closed

Shadow Ban unit tests are flaky due to random sleep times #8150

anoadragon453 opened this issue Aug 24, 2020 · 3 comments · Fixed by #8152
Assignees
Labels
A-Testing Issues related to testing in complement, synapse, etc z-bug (Deprecated Label)

Comments

@anoadragon453
Copy link
Member

Description

The ShadowBanTestCase is flaky:

Likely due to requests sometimes being artificially slowed down to thwart malicious users:

# We randomly sleep a bit just to annoy the requester.
await self.clock.sleep(random.randint(1, 10))

These tests seem to be hitting this timeout, which the containing function has an arbitrary timeout value of 100 (which I believe means 10.0s). That means if a request takes >1s and we had a random sleep of 9s, the request will timeout.

Solutions here I can think of:

  1. Upping this limit to something slightly higher (not sure what else relies on this though)
  2. Weaving the timeout value through a bunch of functions all the way up to self.render such that these tests can explicitly say they expect to take slightly longer
  3. Having a decorator, @override_timeout(150) similar to @override_config. This would be nice, though I'm not sure how exactly to pull it off
@babolivier babolivier added z-bug (Deprecated Label) p1 labels Aug 24, 2020
@clokep
Copy link
Member

clokep commented Aug 24, 2020

Thanks for investigating this a bit! I'll take this since I'm still working on this code anyway. 😄

@clokep clokep self-assigned this Aug 24, 2020
@clokep
Copy link
Member

clokep commented Aug 24, 2020

Solutions here I can think of:

  1. Upping this limit to something slightly higher (not sure what else relies on this though)
  2. Weaving the timeout value through a bunch of functions all the way up to self.render such that these tests can explicitly say they expect to take slightly longer
  3. Having a decorator, @override_timeout(150) similar to @override_config. This would be nice, though I'm not sure how exactly to pull it off

Another option would be to patch random.randint to always return 0 for those tests or something. I think that's my running contender right now, unless we'd benefit from having the timeout be configurable anyway.

@anoadragon453
Copy link
Member Author

anoadragon453 commented Aug 24, 2020

@clokep Perhaps instead we should make a function that generates timeouts for ShadowBanned users and mock that during tests? Something may rely on random.randint now or in future.

Edit: We're only mocking across a single TestCase instead of all tests, so there is no concern.

@richvdh richvdh added the A-Testing Issues related to testing in complement, synapse, etc label Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Testing Issues related to testing in complement, synapse, etc z-bug (Deprecated Label)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants