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 registering a user #15

Merged
merged 8 commits into from
Feb 19, 2014
Merged

Fix registering a user #15

merged 8 commits into from
Feb 19, 2014

Conversation

cyli
Copy link
Member

@cyli cyli commented Sep 7, 2013

An attempt to address #14.

Fixes old AMP usage and adds a sorta test to make sure that q2qclient.UserAdder can successfully callRemote to q2qstandalone.IdentityAdmin.

Tried to use loopbackAsync, but IdentityAdmin depends on having a transport that has a method getQ2QHost, and loopbackAsync uses its own transport, which does not have such a method, and the transport doesn't get exposed for me to patch it. So using twisted.test.iosim instead.

Also included a dependency on https://github.com/alex/pretend for testing.

Edit: any fixes to q2q client are superceded by #16, so remove all fixes in this PR.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8b376b9 on q2q_client_AddUser into ed19930 on master.

# Copyright 2013 Twisted Matrix Laboratories. See LICENSE file for details

"""
Tests for the AddUser (AMP Command) responder and client parts of vertex.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This describes the current content of this file, but the current test coverage is poor, so more is going to be added. I think the docstring should describe what should go in this file, rather than what is currently in this file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that it is tests for the user-registration.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 3067a08 on q2q_client_AddUser into ed19930 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2670e63 on q2q_client_AddUser into b79c858 on master.

'username': username,
'password': password})

store = stub(addUser=addUser)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not used pretend before, but this looks like pretend.call_recorder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, you are right. addUser can just be call_recorder

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 380435d on q2q_client_AddUser into b79c858 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a929194 on q2q_client_AddUser into b79c858 on master.

from vertex.q2q import Q2QAddress
from vertex.q2qadmin import AddUser
from vertex.q2qstandalone import IdentityAdmin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well follow the Twisted coding standard here, 3-2-1

@glyph
Copy link
Member

glyph commented Feb 19, 2014

Thanks for improving the test coverage. My comments notwithstanding I think we should just merge this now.

glyph added a commit that referenced this pull request Feb 19, 2014
@glyph glyph merged commit 88fb1e7 into master Feb 19, 2014
@glyph glyph deleted the q2q_client_AddUser branch February 19, 2014 06:09
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

4 participants