Skip to content

Conversation

@fjetter
Copy link
Member

@fjetter fjetter commented Feb 10, 2022

Not entirely sure if this is connected to our stability issues but I don't see a reason why we would want to lower the connect timeout other than decreasing test runtimes. On CI I'm not too worried about a few seconds more runtime considering this should increase test robustness.

Tests which are actively relying on this timeout to expire (e.g. dead workers are part of the test) should specifically lower this and make their intention clear.

I'm also open to removing this entirely but I could see the point of having smaller timeouts when running locally.

@fjetter fjetter added the flaky test Intermittent failures on CI. label Feb 10, 2022
@fjetter
Copy link
Member Author

fjetter commented Feb 10, 2022

Interesting, the failure rate increased dramatically

@crusaderky
Copy link
Collaborator

What's even the benefit of reducing the timeout locally? Shouldn't we just remove the whole thing?

@crusaderky
Copy link
Collaborator

crusaderky commented Feb 10, 2022

Interesting, the failure rate increased dramatically

Sounds like they may be all tests that expect the connection to fail before the gen_test 30s timeout expires. I think they should be all reviewed. Actually, I would like to set the connection timeout to 240s temporarily and see if more tests start falling over or become unreasonably slow to complete.

@fjetter
Copy link
Member Author

fjetter commented Feb 10, 2022

What's even the benefit of reducing the timeout locally? Shouldn't we just remove the whole thing?

If anything it's about having faster tests. However, I do dislike this approach quite a bit and think that tests should be selective with this. Most tests should not care at all and those who do should be explicit

Sounds like they may be all tests that expect the connection to fail before the gen_test 30s timeout expires. I think they should be all reviewed. Actually, I would like to set the connection timeout to 240s temporarily and see if more tests start falling over or become unreasonably slow to complete.

I'll push a commit and we'll see 👀

@fjetter fjetter changed the title Lower connect timeout only for local test execution [DNM] Lower connect timeout only for local test execution Feb 10, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2022

Unit Test Results

       12 files  ±    0         12 suites  ±0   7h 48m 24s ⏱️ + 41m 52s
  2 620 tests ±    0    2 527 ✔️  -   13    80 💤 ±  0  13 +13 
14 906 runs   - 738  14 036 ✔️  - 745  823 💤  - 40  47 +47 

For more details on these failures, see this check.

Results for commit c69bff1. ± Comparison against base commit fb8484e.

♻️ This comment has been updated with latest results.

@fjetter fjetter force-pushed the connect_timeout_ci_local branch from fde433c to c1f853d Compare February 22, 2022 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky test Intermittent failures on CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants