txawsteam team mailing list archive
-
txawsteam team
-
Mailing list archive
-
Message #00076
Re: [Merge] lp:~oubiwann/txaws/416109-arbitrary-endpoints into lp:txaws
Review: Approve
[1]
+ def get_s3_client(self):
+ raise NotImplementedError
+
+ def get_simpledb_client(self):
+ raise NotImplementedError
+
+ def get_sqs_client(self):
+ raise NotImplementedError
I think you should leave these out since they don't add any useful
functionality.
[2]
You have:
+ self.endpoint = endpoint or self.get_uri()
and then a bit later:
def get_uri(self):
- return self.root_uri + self.get_uri_path()
+ self.endpoint.set_path(self.get_path())
+ return self.endpoint.get_uri()
When endpoint is None, the default situation, an AttributeError will
be raised because self.endpoint is not defined when get_uri tries to
access it. It looks like get_uri should be instantiating and
returning a new endpoint instance, instead of mutating the existing
one. It would be good to add a test to ensure defaults are handled
properly.
[3]
+ def stash_environ(self):
+ self.orig_environ = dict(os.environ)
+ self.addCleanup(self.restore_environ)
+ if 'AWS_ACCESS_KEY_ID' in os.environ:
+ del os.environ['AWS_ACCESS_KEY_ID']
+ if 'AWS_SECRET_ACCESS_KEY' in os.environ:
+ del os.environ['AWS_SECRET_ACCESS_KEY']
+
+ def restore_environ(self):
+ for key in set(os.environ) - set(self.orig_environ):
+ del os.environ[key]
+ os.environ.update(self.orig_environ)
I think these methods should be private. It's not safe for a user
to call stash_environ during a test because it will rewrite
self.orig_environ with the test environment, defeating the purpose
of this code.
If you have already seen it you may want to check out
https://launchpad.net/testresources. It provides a nice framework
for writing helpers that provide per-test services. It could be
useful for doing the kind of thing you're doing above (though maybe
testscenarios is more relevant here?).
[4]
+ def __init__(self, creds, endpoint, instances=[]):
The default list value here will be shared by all FakeEC2Client
instances, which could cause tricky bugs. I recommend you use None
as the default here along with 'self.instances = instances or []'.
[5]
+ def setUp(self):
+ self.addCleanup(self.clean_environment)
+
+ def clean_environment(self):
+ if os.environ.has_key(ENV_ACCESS_KEY):
+ del os.environ[ENV_ACCESS_KEY]
+ if os.environ.has_key(ENV_SECRET_KEY):
+ del os.environ[ENV_SECRET_KEY]
This code is unnecessary given the {stash,restore}_environ methods
in TXAWSTestCase.
Looks good, +1!
--
https://code.edge.launchpad.net/~oubiwann/txaws/416109-arbitrary-endpoints/+merge/10671
Your team txAWS Team is subscribed to branch lp:txaws.
References