Auto init #105
Auto init #105
Conversation
…f options are None.
firebase_admin/__init__.py
Outdated
| @@ -31,7 +32,8 @@ | |||
| _clock = datetime.datetime.utcnow | |||
|
|
|||
| _DEFAULT_APP_NAME = '[DEFAULT]' | |||
|
|
|||
| _CONFIG_JSON_ENV = 'FIREBASE_CONFIG' | |||
There was a problem hiding this comment.
Rename to _FIREBASE_CONFIG_ENV_VAR
firebase_admin/__init__.py
Outdated
| @@ -31,7 +32,8 @@ | |||
| _clock = datetime.datetime.utcnow | |||
|
|
|||
| _DEFAULT_APP_NAME = '[DEFAULT]' | |||
|
|
|||
| _CONFIG_JSON_ENV = 'FIREBASE_CONFIG' | |||
| _CONFIG_VALID_KEYS = ['databaseAuthVariableOverride', 'databaseURL', 'projectId', 'storageBucket'] | |||
There was a problem hiding this comment.
I suppose this should also include httpTimeout.
firebase_admin/__init__.py
Outdated
| raise ValueError('Illegal Firebase app options type: {0}. Options ' | ||
| 'must be a dictionary.'.format(type(options))) | ||
| self._options = options | ||
| if options is not None: |
There was a problem hiding this comment.
This looks a bit messy. Please refactor as follows:
if options is None:
options = _load_from_environment()
...
tests/test_app.py
Outdated
|
|
||
| # This fixture will ignore the environment variable pointing to the default | ||
| # configuration for the duration of the tests. | ||
| @pytest.fixture(scope="session", autouse=True) |
There was a problem hiding this comment.
What are the tests affected by this fixture? Ideally it shouldn't affect tests in other modules.
There was a problem hiding this comment.
yes you are right, this is no longer needed.
tests/test_app.py
Outdated
| @@ -102,6 +264,10 @@ class TestFirebaseApp(object): | |||
| firebase_admin.App('uninitialized', CREDENTIAL, {}) | |||
| ] | |||
|
|
|||
| bad_config_file = ['firebase_config_empty.json', | |||
There was a problem hiding this comment.
Inline this list wherever you're using it. Seems to be used in only one test case.
tests/test_app.py
Outdated
| @@ -135,11 +301,29 @@ def test_app_init_with_invalid_options(self, options): | |||
| with pytest.raises(ValueError): | |||
| firebase_admin.initialize_app(CREDENTIAL, options=options) | |||
|
|
|||
| @pytest.mark.parametrize('bad_file_name', bad_config_file) | |||
| def test_default_app_init_with_bad_config_from_env(self, bad_file_name): | |||
There was a problem hiding this comment.
test_app_init_with_invalid_config_file
tests/test_app.py
Outdated
| with pytest.raises(ValueError): | ||
| firebase_admin.initialize_app(CREDENTIAL) | ||
| finally: | ||
| if config_file_old: |
There was a problem hiding this comment.
I think you should create a fixture, that sets a given value/file as the FIREBASE_CONFIG variable, and then reverts it back after the test. Then inject that fixture to any tests that is potentially affected by this change. You can probably get rid of that session scoped/autouse fixture that way.
There was a problem hiding this comment.
This is exactly what I was doing in the OptionsTest class,
nm, PTAL
tests/test_app.py
Outdated
| return dict(zip(("ids", "params"), zip(*named_id_option_pairs))) | ||
|
|
||
|
|
||
| @pytest.fixture(**named_option_pairs_for_test([ |
There was a problem hiding this comment.
This is a readability nightmare. Can we turn these into individual test cases? At very least, set this dictionary up as a global constant. Then use pytest indirect parameters to pass the test parameters from test case to the fixture: https://docs.pytest.org/en/latest/example/parametrize.html#apply-indirect-on-particular-arguments
…. refactor env var setting
firebase_admin/__init__.py
Outdated
| if options is None: | ||
| options = {} | ||
| if not isinstance(options, dict): | ||
| self._load_from_environment() |
There was a problem hiding this comment.
Make this function side-effect free. I think the overall logic should look like:
if options is None:
options = self._load_from_environment()
if not isinstance(options, dict):
# error
self._options = options
firebase_admin/__init__.py
Outdated
| 'must be a dictionary.'.format(type(options))) | ||
| self._options = options | ||
| 'must be a dictionary.'.format(type(self._options))) | ||
|
|
There was a problem hiding this comment.
Remove redundant empty line
firebase_admin/__init__.py
Outdated
|
|
||
| def get(self, key, default=None): | ||
| """Returns the option identified by the provided key.""" | ||
| return self._options.get(key, default) | ||
|
|
||
| def _load_from_environment(self): |
There was a problem hiding this comment.
Return a dict from this method, rather than mutating the state.
| raise ValueError('Unable to read file {}. {}'.format(config_file, err)) | ||
| try: | ||
| json_data = json.loads(json_str) | ||
| except Exception as err: |
There was a problem hiding this comment.
Can we not handle this exception, and just let it be thrown?
There was a problem hiding this comment.
we can,
the error message is "Expecting property name: line 1 column 2 (char 1)"
I think it is clearer if we add information, but willing to remove if you want it gone.
tests/test_app.py
Outdated
| revert_config_env(config_old) | ||
|
|
||
|
|
||
| OptionsTestCase = namedtuple('OptionsTestCase', |
There was a problem hiding this comment.
This is sitting in the middle of a class definition. Please move to somewhere more appropriate.
tests/test_app.py
Outdated
| app = firebase_admin.initialize_app(CREDENTIAL, options=test_case.init_options) | ||
| for field in firebase_admin._CONFIG_VALID_KEYS: | ||
| assert app.options.get(field) == test_case.want_options.get(field), test_case.name | ||
| revert_config_env(config_old) |
There was a problem hiding this comment.
This won't run if an assertion fails. See if you can turn it into a self-cleaning fixture like https://github.com/firebase/firebase-admin-python/blob/master/tests/test_credentials.py#L83
If that's not possible, at least use a try-finally block.
There was a problem hiding this comment.
I've put it in the fixture according to your first suggestion, thanks.
PTAL
tests/test_app.py
Outdated
| 'projectId': 'pid1 - mock', | ||
| 'storageBucket': 'sb1 - .mock'})] | ||
|
|
||
| @pytest.mark.parametrize('test_case', options_test_cases) |
There was a problem hiding this comment.
Pass ids= parameter here. Then you don't have to use test_case.name in the body of the test case.
tests/test_app.py
Outdated
| OptionsTestCase = namedtuple('OptionsTestCase', | ||
| 'name, config_json, init_options, want_options') | ||
| options_test_cases = [ | ||
| OptionsTestCase(name='no env var, empty options', |
There was a problem hiding this comment.
Can you clean up/clarify the test case names a bit? Perhaps get some ideas from the Java PR?
firebase_admin/__init__.py
Outdated
| @@ -149,12 +149,12 @@ class _AppOptions(object): | |||
| def __init__(self, options): | |||
| self._options = options | |||
There was a problem hiding this comment.
Remove this line, now that we're doing the assignment below.
tests/test_app.py
Outdated
| @pytest.fixture(scope="function") | ||
| def env_test_case(request): | ||
| config_old = set_config_env(request.param.config_json) | ||
| def fin(): |
There was a problem hiding this comment.
Can we use yield here? http://pytest.readthedocs.io/en/2.8.7/yieldfixture.html
tests/test_app.py
Outdated
| for field in firebase_admin._CONFIG_VALID_KEYS: | ||
| assert app.options.get(field) == test_case.want_options.get(field), test_case.name | ||
| revert_config_env(config_old) | ||
| assert app.options.get(field) == env_test_case.want_options.get(field) |
There was a problem hiding this comment.
Can we do a stronger check here? Something like assert app.options._options == env_test_cast.want_options?
| ``databaseURL``, ``storageBucket``, ``projectId``, ``databaseAuthVariableOverride`` | ||
| and ``httpTimeout``. If ``httpTimeout`` is not set, HTTP connections initiated by client | ||
| modules such as ``db`` will not time out. | ||
| If options are not provided an attempt is made to load the options from the environment. |
There was a problem hiding this comment.
This should be part of the method description, rather than the argument description.
Adding the auto init from env var to python.