Fixes lp#1589641: ActionSuite.TestUnitWatchActionNotifications unexpected change. #6574

Merged
merged 2 commits into from Nov 23, 2016

Conversation

Projects
None yet
4 participants
Member

anastasiamac commented Nov 17, 2016

Under race & stress testing, reads of changes from watcher chan seem to get out of order.
The watcher chan is monitoring testing State which seems to get out of sync. This propose syncs testing state.

QA:
Before code change, under stress, with race flag, this test would fail within the 1st 170 runs.
After this code change, the test has not failed after 5391 successful runs over 232 min 15 sec.

state/testing/watcher.go
+ // Fixes lp#1589641: some time, under race & stress testing,
+ // reads of changes from watcher chan seem to get out of order.
+ // This additional Sync, ensures that the changes are processed correctly.
+ c.State.StartSync()
@jameinel

jameinel Nov 17, 2016

Owner

This seems overbroad, as things should be doing some sort of StartSync in the function themselves, rather than as a side effect of AssertChange.
Is there a specific test which is failing? Or is this a group of tests that are all missing the synchronization?

@anastasiamac

anastasiamac Nov 17, 2016

Member

Specific test that is failing is in the title - TestUnitWatchActionNotifications
However, we have other watcher tests with stringwatchers that seem to exhibit similar behavior.

Owner

jameinel commented Nov 17, 2016

Can you clarify what actual test is failing? As this is a simple "AssertChange" helper, vs being an actual TestCase that is failing. (the correct fix feels like it should be adding the StartSync to the failing test, rather than inside StringsWatcherC.AssertChange)

Owner

jameinel commented Nov 22, 2016

LGTM

LGTM too!

Member

anastasiamac commented Nov 23, 2016

$$merge$$

Contributor

jujubot commented Nov 23, 2016

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

@jujubot jujubot merged commit 8dc9e57 into juju:develop Nov 23, 2016

1 check passed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@anastasiamac anastasiamac deleted the anastasiamac:actionsuite-lp1589641 branch Nov 23, 2016

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