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

Make Violation messages for arguments and answers include the method name #12

Merged
merged 1 commit into from Sep 29, 2013

Conversation

daira
Copy link
Contributor

@daira daira commented Nov 9, 2012

Make Violation messages for arguments and answers include the method name. fixes #201

Signed-off-by: David-Sarah Hopwood david-sarah@jacaranda.org

…name. fixes #201

Signed-off-by: David-Sarah Hopwood <david-sarah@jacaranda.org>
@warner
Copy link
Owner

warner commented Aug 29, 2013

Finally digging into this. The basic patch looks good, although it makes the tests fail (test_appserver, test_banana.TestCall, test_keepalive, at least). Probably something shallow. I'd like to add a test for the new functionality too.

@warner
Copy link
Owner

warner commented Aug 29, 2013

Ah, yeah, it was shallow: Brokers do not always have a methodSchema, and this is exercised by the unit tests. I pushed a "pr12" branch with the additional fix. The next chance I get (or if someone else beats me to it :), I'll add a proper test.

@daira
Copy link
Contributor Author

daira commented Aug 30, 2013

Thanks!

@warner
Copy link
Owner

warner commented Aug 30, 2013

Hm, this is tricky to test. The first change requires us to provoke a failure during outbound serialization of the arguments (but after the static schema check has passed, so it's not as simple as violating the schema). The second change requires a failure to occur during the remote serialization of the response, again after the static schema check.

One trick that might help would be an object with custom serialization logic, which returns one kind of value the first time it is queried, and a different (failing) value the second time.

@warner warner merged commit 11af15d into warner:master Sep 29, 2013
@warner
Copy link
Owner

warner commented Sep 29, 2013

I've landed this, but it'd still be nice to get some explicit tests for it at some point.

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.

None yet

2 participants