/juju

worker/logforwarder: Make tests reliable #6456

Merged
merged 2 commits into from Oct 17, 2016

Conversation

Reviewers

@howbazaar howbazaar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mjs @jujubot @howbazaar
@mjs
Contributor

mjs commented Oct 16, 2016

The tests were using a convoluted mess of channels in an attempt to synchronise the multiple goroutines being tested with the test code. This was hard to understand and work with and is likely to be
caused the observed intermittent test failures.

Changes:

  • use independent stubs for the stream and sender to make it easier to assert calls and to have faked errors always be returned by the correct instance.
  • use a single simpler "activity" channel in the stub sender to synchronise test and code-under-test progress
  • make checks more reliable by only checking stubs once worker has shut down.
  • removed unnecessary record sending goroutine in stub stream (just stack a buffered channel instead.

Likely fix for https://bugs.launchpad.net/juju/+bug/1606568

QA

Many runs of the test suite with the race stress tester. This is a unit test only change so there's other QA to do.

Jenkins bot and others added some commits Oct 14, 2016

Merge commit 'b477750233656fc00316d1c03a3467839949f24f' into HEAD
df0effe
@mjs
worker/logforwarder: Make tests reliable 
The tests were using a convoluted mess of channels in an attempt to
synchronise the multiple goroutines being tested with the test
code. This was hard to understand and work with and is likely to be
caused the observed intermittent test failures.

Changes:
- use independent stubs for the stream and sender to make it easier to
  assert calls and to have faked errors always be returned by the
  correct instance.
- use a single simpler "activity" channel in the stub sender to
  synchronise test and code-under-test progress
- make checks more reliable by only checking stubs once worker has shut
  down.
- removed unnecessary record sending goroutine in stub stream (just
  stack a buffered channel instead.

Likely fix for https://bugs.launchpad.net/juju/+bug/1606568
43f0d8f
@mjs
Contributor

mjs commented Oct 17, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Oct 17, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 3c63cd8 into juju:develop Oct 17, 2016
1 check passed

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy.
Details

@mjs mjs deleted the mjs:1606568-TestStreamError branch Oct 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment