Merge lp://staging/~jelmer/brz/fix-flaky into lp://staging/brz

Proposed by Jelmer Vernooij
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: no longer in the source branch.
Merge reported by: The Breezy Bot
Merged at revision: not available
Proposed branch: lp://staging/~jelmer/brz/fix-flaky
Merge into: lp://staging/brz
Diff against target: 27 lines (+7/-3)
1 file modified
breezy/tests/test_test_server.py (+7/-3)
To merge this branch: bzr merge lp://staging/~jelmer/brz/fix-flaky
Reviewer Review Type Date Requested Status
Martin Packman Approve
Review via email: mp+355057@code.staging.launchpad.net

Commit message

Cope with very early disconnects during TCP Server tests.

Description of the change

Cope with very early disconnects during TCP Server tests.

The debian packages have carried these changes as patches for a while.

To post a comment you must log in.
Revision history for this message
Martin Packman (gz) wrote :

Okay.

review: Approve
Revision history for this message
The Breezy Bot (the-breezy-bot) wrote :

Running landing tests failed
https://ci.breezy-vcs.org/job/land-brz/492/

Revision history for this message
Vincent Ladeuil (vila) wrote :

Err, no

Please.

Disable those tests if you wish but if you want to fix them it's either:
- the easy way: no server and client sharing a socket in the same process
- the hard way: fix races when using server and client sharing a socket in the same process

You won't get the later by peppering blindly.

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

This test has always been broken. We can either:

1) Remove the test altogether
2) Make the test just raise KnownFailure
3) The current solution

I don't have particularly strong opinion on what we do here, but we should somehow get rid of this error.

The current approach at least means we find out about the regression that the test checks for, even if we sometimes effectively don't run the test. We take a similar approach on test_test_server.py line 195

Revision history for this message
Vincent Ladeuil (vila) wrote :

Yup, that approach is sound even if the price is a reduced coverage.

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

On Tue, Sep 18, 2018 at 08:37:26AM -0000, Vincent Ladeuil wrote:
> Yup, that approach is sound even if the price is a reduced coverage.
Which of the three approaches, just getting rid of the test?

--
Jelmer Vernooń≥ <email address hidden>
PGP Key: https://www.jelmer.uk/D729A457.asc

Revision history for this message
Vincent Ladeuil (vila) wrote :

The one proposed here: b'', client.read().

But I don't think the original writing of that test implied having to do that, so the coverage is reduced.

Removing the test altogether is worse.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches