txawsteam team mailing list archive

[Jamu Kakar <jkakar@xxxxxxxx>]

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.