Opened 9 years ago

Closed 9 years ago

#3999 defect closed fixed (fixed)

AMP callRemoteString ignores requiresAnswer

Reported by: ivank Owned by: jesstess
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess Branch:
Author: jesstess

Description (last modified by Jean-Paul Calderone)

BoxDispatcher.callRemoteString has a requiresAnswer argument (and documentation), but it is not passed to return self._sendBoxCommand(command, box). There are also no tests for the case where requiresAnswer=False.

Attachments (1)

amp_callRemoteString_requiresAnwerFalse.patch (1.2 KB) - added by jesstess 9 years ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 9 years ago by Jean-Paul Calderone

Description: modified (diff)

Fixing description markup.

comment:2 Changed 9 years ago by jesstess

Cc: jesstess added
Keywords: review added
Owner: Glyph deleted

The patch adds requiresAnswer to the call to _sendBoxCommand and adds a test case in twisted/test/ that callRemoteString returns None if requiresAnswer is False.

Something else came up while looking into this ticket:

It looks like callRemoteString creates a new AmpBox that it passes to _sendBoxCommand, and when you make requiresAnswer False, in _sendBoxCommand only a _command tag gets added to this new box before calling box._sendTo(self.boxSender).

However, the AmpBox._sendTo docstring states that exactly one of _ask, _answer, and _error is a required key.

Is the docstring incorrect, or is the path that returnFalse forces bad? Other tests in, including test_requiresNoAnswer and test_requiresNoAnswerFail, produce AmpBoxes without one of the required keys (ostensibly _ask should be set, as _command is required if _ask is present).

print statements from AmpBox._sendBox while running the tests:

test_callRemoteStringRequiresAnswerFalse ... AmpBox({'_command': 'WTF'})


test_requiresNoAnswer ... AmpBox({'hello': 'world', '_command': 'hello'})


test_requiresNoAnswerFail ... AmpBox({'hello': 'fuck you', '_command': 'hello'})


comment:3 Changed 9 years ago by therve

Keywords: review removed
Owner: set to jesstess

Nice patch! Just one comment: please use assertIdentical to compare to None.

Please apply then.

Changed 9 years ago by jesstess

comment:4 Changed 9 years ago by jesstess

Author: jesstess
Resolution: fixed
Status: newclosed

(In [27346]) Apply amp_callRemoteString_requiresAnwerFalse.patch

Author: jesstess Reviewer: therve Fixes: #3999

Have BoxDispatcher.callRemoteString pass its requiresAnswer argument to its call to _sendBoxCommand. Also add a unit test for requiresAnswer = False.

Note: See TracTickets for help on using tickets.