Code review comment for lp:~oubiwann/txaws/416109-arbitrary-endpoints

[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!