Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #686 for circusctl as well (refactored) #932

Merged

Conversation

StefanWiechula
Copy link
Contributor

Issue #686 was partially fixed by disabling the decr link in circus-web but the problematic behaviour is still accessible through circusctl decr and presumably raw ZMQ.

This replaces PR #931.

Just like 931 it:

  • Renames DecrProcess to DecrProc for consistency with IncrProc
  • Implements the same singleton checking logic as IncrProc in DecrProc
  • Adds tests for the correct behaviour in both commands given singleton watchers

Contrary to 931, to address @douardda's comment on 931, it:

  • Spreads the changes across 4 commits instead of 2
  • Uses a subclass of FakeArbiter to support singleton FakeWatchers, instead of refactoring FakeArbiter to support dependency injection of the watcher class. This is similar to how test_command_set.py subclasses FakeArbiter and does not affect any of the other test files that import FakeArbiter from test_command_incr.py.

Match the name of the corresponding attribute on real Watcher instances.
This will be important when adding tests for code that accesses this
attribute (e.g. any test that covers commands/incrproc.py line 64).
Note that the decr command fails this test while incr passes the
analogous test already.
The decr command now passes test added in 0e8982f.
@douardda
Copy link

Sorry for the very long delay. This PR looks fine for me, let's merge it (I'll add a small patch to please flake8 as well).

douardda pushed a commit that referenced this pull request Jan 12, 2016
…tored

Fix issue #686 for circusctl as well (refactored)
@douardda douardda merged commit 9747abd into circus-tent:master Jan 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants