lp:~free.ekanayaka/landscape-client/amp-test-readability

Branch merges

Propose for merging

No branches dependent on this one.

Merged into lp:~landscape/landscape-client/trunk at revision 674

Chris Glass (community): Approve on 2013-05-14

Christopher Armstrong (community): Approve on 2013-05-13

Diff: 781 lines (+168/-275)

2 files modified

landscape/lib/amp.py (+5/-5)
landscape/lib/tests/test_amp.py (+163/-270)

api

Related source package recipes

No recipes using this branch. (?)

Create packaging recipe

Related snap packages

Related bugs

Bug #1170669: Cleanup AMP-based machinery

High

Fix Released

Link a bug report

Related blueprints

674. By Free Ekanayaka on 2013-05-13

Drop Words class

673. By Christopher Armstrong on 2013-05-09

Merge retry-package-ops [f=1177419] [r=free.ekanayaka,jseutter] [a=Christopher Armstrong]
Package installation failures, including failures to acquire the apt lock, are now retried a number of times with a delay in between.

672. By Free Ekanayaka on 2013-05-07

Merge amp-cleanup-9 [f=1170669] [r=radix,tribaal] [a=Free Ekanayaka]
Alright, one of the last episodes of the AMP cleanup saga. In this branch:

- Drop landscape.lib.amp.RemoteObjectConnector, as most of its functionality has no been pushed down to MethodCallClientFactory which is a more standard Twisted primitive more importantly and is transport-agnostic, making landscape.lib.amp really generic.

- Convert landscape.lib.tests.test_amp.RemoteObjectConnectorTest to MethodCallFunctionalTest. The RemoteObjectConnectorTest class was exercising the (now gone) RemoteObjectConnector in a functional-like way, that means spinning the real twisted reactor and listening/connecting to real Unix sockets. Even if from a strict unit-test point of view all the functionality in landscape.lib.amp is already tested, I felt it was nice to still have some functional-like coverage mainly to protect us against regressions or API changes in the Twisted machinery that we depend on. I've used @inlineCallbacks to make tests a bit more readable.

671. By Chris Glass on 2013-05-07

Rephrased comment. [trivial]

670. By Free Ekanayaka on 2013-05-06

Merge amp-cleanup-8 [f=1170669] [r=tealeg,ack] [a=Free Ekanayaka]
Some more cleanup around the AMP-based machinery:

- landscape.amp.ComponentConnector: the disconnect method now uses Twisted's IConnector.disconnect(), without messing with the lower-level ITransport.loseConnection(). The objects implementing IConnector are the ones returned by reactor.connectUNIX/connectTCP/connectSSH and afaiu they're what is supposed to be used for terminating a connection.

- rename RemoteComponentRegistry to ComponentRegistry, to match ComponentPublisher and ComponentConnector

- landscape.configuration: the changes here are due because we know use IConnector.disconnect() vs ITransport.loseConnection(). Unfortunately the code in this module is factored and tested in a far less than ideal way, eventually we should take time to clean it up.

- landscape.lib.amp.RemoteObject: update the constructor docstring and reduce a bit of logic duplication by adding a new _send_method_call helper method, which is now used by __getattr__ and _retry (this makes _retry flush the fake asynchronous connection used in tests, in the same way __getattr__ does).

- landscape.lib.amp.MethodCallServerFactory: rename the "reactor" init parameter to "clock", since only the IReactorTime interface is needed (in this case instances are normally called "clock" and can be mocked with a twisted.internet.task.Clock instance in tests)

- landscape.lib.tests.test_amp: Add a FakeConnector class implementing IConnector, so we can use it unit-tests. Also, I've converted a few deferred-returning (asynchronous) tests to synchronous equivalent.

- landscape.tests.test_amp: Concerted asynchronous tests to synchronous equivalent.

669. By Christopher Armstrong on 2013-05-03

Merge reboot-after-message [r=free.ekanayaka,ack] [f=1176121]

Delay rebooting until *after* the package operation has been responded to
(plus 5 seconds for safety). This avoids a race condition where the "shutdown"
command would kill landscape-client before the message could be saved to disk
by the broker.

668. By Free Ekanayaka on 2013-05-03

Drop our copy of initgroups.c as it's only relevant for Python < 2.7 and it's fixed in Lucid's Twisted which uses 2.6 [trivial] [r=ack]

667. By Free Ekanayaka on 2013-05-02

Merge amp-cleanup-7 [f=1170669] [r=tribaal,tealeg] [a=Free Ekanayaka]
This branch is simplifies the setup methods of all our test helpers and test cases using AMP connections, by not making them return deferreds (and hence making twisted.trial.unittest.TestCase eventually spin the reactor, see http://twistedmatrix.com/documents/current/core/howto/trial.html#auto7).

666. By Free Ekanayaka on 2013-05-02

Merge amp-cleanup-6 [f=1170669] [r=tribaal,radix,tealeg] [a=Free Ekanayaka]
Another episode of the AMP-related cleanup:

- Rename landscape.amp.RemoteComponentConnector to simply ComponentConnector (the fact that it's remote is implied by the fact that you connect to it, and the new name complements the existing ComponentPublisher nicely)

- Drop the landscape.amp.ComponentProtocolClientFactory sub-class and use MethodCallClientFactory directly, tweaking the needed instance attributes with explicit assignements (this is a sort of composition-over-inheritance change)

- Drop landscape.reactor.UnixReactorMixin. The landscape.reactor.FakeReactor class now really fakes the Unix socket transport, by re-using the machinery in landscape.lib.tests.test_amp, instead of delegating the behavior to the real twisted reactor (hence making it not fake at all)

- Update tests that were using FakeReactor.listen_unix/connect_unix (directly or via helpers) to be synchronous. This means essentially using FakeReactor.advance to simulate time progresses and trigger scheduled calls.

- Make landscape.lib.amp.RemoteObject aware of fake transports, so it can flush it transparently. This is a hacky workaround to the fact that twisted's AMP code doesn't like synchronous transports, and without it we'd need to explicitly flush the fake transport buffers explicitly all the times in a test we communicate with a remote component connected via a fake transport.

- Drop landscape.service.LandscapeService.socket instance attribute, as LandscapeService-derived classes now all use the new landscape.amp.ComponentPublisher convenience introduced in former branches, that takes care of that.

665. By Free Ekanayaka on 2013-04-30

Merge amp-cleanup-5 [f=1170669] [r=radix,tealeg] [a=Free Ekanayaka]
This branch continues the AMP machinery cleanup started in former branches. In particular:

- Drop the landscape.amp.ComponentProtocol sub-class and all its sub-classes, which is now not needed since the methods to be exposed are now passed to landscape.amp.ComponentPublisher

- Rename ComponentProtocolFactory to ComponentProtocolClientFactory, since we now use 2 different factories for the client and server sides (they are asymmetric and implement different logic)

- Use a uuid instead of a sequence number to track in-progress MethodCall. This fixes a latent bug that probably never manifested itself (i.e. monitor and manager connecting to the broker and independently sending the same sequence number for different MethodCalls)

- Drop MethodCallProtocol which has become unused, along with some other temporary code not used anymore.