Launchpad itself

Merge ~ines-almeida/launchpad:increase-unittest-fetch-timeout into launchpad:master

  1. Git
  2. lp:~ines-almeida/launchpad
  3. increase-unittest-fetch-timeout
  4. Merge into master
Proposed by Ines Almeida
Status: Rejected
Rejected by: Ines Almeida
Proposed branch: ~ines-almeida/launchpad:increase-unittest-fetch-timeout
Merge into: launchpad:master
Diff against target: 15 lines (+3/-1)
1 file modified
lib/lp/services/tests/test_timeout.py (+3/-1)
Reviewer Review Type Date Requested Status
Jürgen Gmach Approve
Review via email: mp+458930@code.launchpad.net

Commit message

test: increase timeout in test that often times out

test_urlfetch_writes_to_output_file often times out, this increases the timeout for that particular test to double to prevent inconsistent test failures

To post a comment you must log in.
Revision history for this message
Simone Pelosi (pelpsi) wrote :

Is this change related to flaky tests ?

Revision history for this message
Ines Almeida (ines-almeida) wrote :

Yes, this test fails from time to time, and failed twice in recent runs (example, last run http://lpbuildbot.canonical.com/builders/lp-devel-focal/builds/452/steps/shell/logs/summary)

Revision history for this message
Jürgen Gmach (jugmac00) wrote :

I wonder why such a simple test could take more than 30 seconds.

I did not have a closer look, just two points of thoughts:
- Are we ok that our test suite gets longer? It is only another 30 seconds, but still, I think 30 or even 60 seconds for a simple test are pretty crazy. Should we rather check whether we could cut down the time of the test?
- When changing the test, and we are ok when things take longer, does that also mean we are ok when things on production take longer?

Revision history for this message
Ines Almeida (ines-almeida) wrote :

I think we need to be practical: We either need to take the time to improve our test and build systems properly, or we reduce the flakyness test by test - the first is much better, but we currently we are doing neither.

I <b>100%</b> think that we need to take the time to go deep into this. But while we don't, our builds fail more than 50% of the time due to 1 or 2 flaky tests failing. This often brings hours (if not days, sometimes) of delay in deployments.

FWIW, I ran this test with a 0.2 second timeout locally with `--repeat 20` and it always ran successfully (it fails if I set the timeout to 0.1). But when the buildbot runs it, sometimes it somehow takes longer than 30 seconds.
Also FWIW, I don't expect this to add 30 seconds to the test run - it only fails from time to time, so it probably takes only slightly longer than the 30 seconds.

So although this is not the best solution, it's at least something.

Happy to discuss this at stand-up tomorrow!

Revision history for this message
Jürgen Gmach (jugmac00) wrote :

As discussed, happy to have this merged if it improves our situation for now.

As agreed, this is no long term solution.

Please link to that spot in the source code from the spike ticket you created to investigate the flaky tests.

review: Approve
Revision history for this message
Ines Almeida (ines-almeida) wrote :

Ever since we moved the buildbot, this hasn't been an issue. Marking this as rejected

Unmerged commits

14fbea4... by Ines Almeida

test: increase timeout in test that often times out

test_urlfetch_writes_to_output_file often times out, this increases the timeout for that particular test to double to prevent inconsistent test failures

Succeeded
[SUCCEEDED] docs:0 (build)
[SUCCEEDED] lint:0 (build)
[SUCCEEDED] mypy:0 (build)
1 → 3 of 3 results First • Previous • Next • Last

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/services/tests/test_timeout.py b/lib/lp/services/tests/test_timeout.py
2index 0c68401..a776668 100644
3--- a/lib/lp/services/tests/test_timeout.py
4+++ b/lib/lp/services/tests/test_timeout.py
5@@ -510,7 +510,9 @@ class TestTimeout(TestCase):
6 t = threading.Thread(target=success_result)
7 t.start()
8 output_path = self.useFixture(TempDir()).join("out")
9- with open(output_path, "wb+") as f:
10+ # Default timeout (30s) is often not enough for this test.
11+ # We temporarily override timeout to be 60s.
12+ with open(output_path, "wb+") as f, override_timeout(60):
13 urlfetch(http_server_url, output_file=f)
14 f.seek(0)
15 self.assertEqual(b"Success.", f.read())

Subscribers

People subscribed via source and target branches

to status/vote changes: