Opened 5 years ago

Closed 5 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 Launchpad Bug:

Description (last modified by exarkun)

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 5 years ago.

Download all attachments as: .zip

Change History (5)

comment:1 Changed 5 years ago by exarkun

  • Description modified (diff)

Fixing description markup.

comment:2 Changed 5 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/test_amp.py 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
test_amp.py, 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'})

[OK]

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

[OK]

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

[OK]

comment:3 Changed 5 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 5 years ago by jesstess

comment:4 Changed 5 years ago by jesstess

  • Author set to jesstess
  • Resolution set to fixed
  • Status changed from new to closed

(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.