Make metrics deactivable for tests #905
Conversation
|
Nice 👍
I hoped it would be more than that but it's better than nothing :) |
udata/core/metrics/__init__.py
Outdated
| @@ -14,6 +14,7 @@ | |||
|
|
|||
|
|
|||
| from udata.models import db # noqa: need metrics refactoring | |||
| from flask import current_app | |||
There was a problem hiding this comment.
Import with third-parties dependencies above.
udata/core/metrics/__init__.py
Outdated
| @@ -111,10 +112,12 @@ def iso(self, value): | |||
| 'Unsupported format: {0} ({1})'.format(value, type(value))) | |||
|
|
|||
| def trigger_update(self): | |||
| self.need_update.send(self) | |||
| if not current_app.config['TESTING'] or current_app.config.get('FORCE_METRICS'): | |||
There was a problem hiding this comment.
Worth an entry in settings' documentation?
There was a problem hiding this comment.
Yep, it should be documented, but I prefer a USE_METRICS or ENABLE_METRICS instead of force metrics (see next comment)
udata/core/metrics/__init__.py
Outdated
|
|
||
| def notify_update(self): | ||
| self.updated.send(self) | ||
| if not current_app.config['TESTING'] or current_app.config.get('FORCE_METRICS'): |
There was a problem hiding this comment.
I'm not sure we want to restrict that to TESTING, I'm thinking about local load of backups for instance but that might be another issue. @noirbizarre thoughts?
There was a problem hiding this comment.
Instead of testing both TESTING and FORCE_METRICS, I think it would be cleaner to have a single USE_METRICS defined to True in udata.settings.Defaults and overriden to False in udata.settings.Testing so:
- we avoid runtime code to perform to have a view on testing
- we have all testing parameters in the same place
- activating metrics in test is simply setting
config.USE_METRICSto True
@davidbgk I think local load of backups is not performed inside the application so metrics are not triggered.
udata/tests/test_discussions.py
Outdated
| @@ -37,6 +37,7 @@ def create_app(self): | |||
| return app | |||
|
|
|||
| def test_new_discussion(self): | |||
| self.app.config['FORCE_METRICS'] = True | |||
There was a problem hiding this comment.
Why not in the setUp of that class if you have to set it everywhere?
There was a problem hiding this comment.
Agreed 👍 : you should factorize.
Even better, you should write a MetricsSettings class for this case. See https://github.com/opendatateam/udata/blob/master/udata/tests/frontend/test_markdown.py#L16-L21 for an example (it's the pattern we use when we need to activate some settings)
There was a problem hiding this comment.
This was done on purpose, I wanted to activate it as less as possible.
udata/tests/test_issues.py
Outdated
| @@ -37,6 +37,7 @@ def create_app(self): | |||
| return app | |||
|
|
|||
| def test_new_issue(self): | |||
| self.app.config['FORCE_METRICS'] = True | |||
udata/core/metrics/__init__.py
Outdated
|
|
||
| def notify_update(self): | ||
| self.updated.send(self) | ||
| if not current_app.config['TESTING'] or current_app.config.get('FORCE_METRICS'): |
There was a problem hiding this comment.
Instead of testing both TESTING and FORCE_METRICS, I think it would be cleaner to have a single USE_METRICS defined to True in udata.settings.Defaults and overriden to False in udata.settings.Testing so:
- we avoid runtime code to perform to have a view on testing
- we have all testing parameters in the same place
- activating metrics in test is simply setting
config.USE_METRICSto True
@davidbgk I think local load of backups is not performed inside the application so metrics are not triggered.
udata/core/metrics/__init__.py
Outdated
| @@ -111,10 +112,12 @@ def iso(self, value): | |||
| 'Unsupported format: {0} ({1})'.format(value, type(value))) | |||
|
|
|||
| def trigger_update(self): | |||
| self.need_update.send(self) | |||
| if not current_app.config['TESTING'] or current_app.config.get('FORCE_METRICS'): | |||
There was a problem hiding this comment.
Yep, it should be documented, but I prefer a USE_METRICS or ENABLE_METRICS instead of force metrics (see next comment)
udata/tests/test_discussions.py
Outdated
| @@ -37,6 +37,7 @@ def create_app(self): | |||
| return app | |||
|
|
|||
| def test_new_discussion(self): | |||
| self.app.config['FORCE_METRICS'] = True | |||
There was a problem hiding this comment.
Agreed 👍 : you should factorize.
Even better, you should write a MetricsSettings class for this case. See https://github.com/opendatateam/udata/blob/master/udata/tests/frontend/test_markdown.py#L16-L21 for an example (it's the pattern we use when we need to activate some settings)
udata/tests/test_transfer.py
Outdated
| @@ -102,6 +102,9 @@ def test_request_transfer_to_same_organization(self): | |||
|
|
|||
|
|
|||
| class TransferAcceptTest(TestCase, DBTestMixin): | |||
| def setUp(self): | |||
| self.app.config['USE_METRICS'] = True | |||
There was a problem hiding this comment.
Don't forget the super().setUp()
We can observe a gain of 10% on the runtime of tests
l-vincent-l
force-pushed
the
metrics_deactivable
branch
from
5e9d2b1
to
b88911e
Compare
We can observe a gain of 10% on the runtime of tests