-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor MSColab away from a bunch of global variables #2442
Comments
No idea what to tag this as. It isn't a bug per-se, but also not an enhancement. This is an architectural change that should be invisible to a user, and as such doesn't require a major release. I've picked bug for now. The most fitting label would be "technical debt" IMO. |
In the context of #2437 it was found that different locations in code had different instances of SocketManager, for some reason. I think this is a bug resulting from the very convoluted application setup. Some tests also instantiate their own objects of the manager types and then mix interactions with them and the global instances, which again is inconsistent usage of the objects and seems to make some issues in #2437's tests. |
@ReimarBauer this is not fixed, our Flask app instance is still a global object. |
@matrss ok, the further work on that is a deeper API change. I don't remove the bug label, because that API change is more than only an improvement. We can mention this later in CHANGES. |
Currently the MSColab Flask app is defined by a bunch of global variables (the Flask app instance that is imported and reused everywhere, and mscolab_settings mainly). The often seen alternative approach is to have a factory function constructing and returning a Flask instance (like described here: https://flask.palletsprojects.com/en/3.0.x/patterns/appfactories/), which could then be fed with the configuration to build a matching app instance.
This would make it easier to get independent Flask app instances in tests. I suspect that the current construction is one of the reasons why I still see inter-dependencies between tests on the database level.
It would also make it possible to construct differently configured Flask app instances for different tests. This would mainly come in handy to make tests/_test_mscolab/test_server_auth_required.py run as part of an ordinary run of the test suite.
This came up while I am currently struggling with building a test for migrating data from one database to a new one, as part of #2432. Since (almost?) all of the database related helper functions import and use the global app instance it is basically impossible to make them act on a different database.
The text was updated successfully, but these errors were encountered: