gabotorresruiz commented on code in PR #45552: URL: https://github.com/apache/superset/pull/45552#discussion_r4354775003
########## tests/integration_tests/fixtures/public_role.py: ########## @@ -14,48 +14,55 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from collections.abc import Iterator +from contextlib import contextmanager + import pytest from flask.ctx import AppContext +from flask_appbuilder.security.sqla.models import PermissionView from superset.extensions import db, security_manager from tests.integration_tests.test_app import app [email protected] -def public_role_like_gamma(app_context: AppContext): - app.config["PUBLIC_ROLE_LIKE"] = "Gamma" - security_manager.sync_role_definitions() - - yield - - security_manager.get_public_role().permissions = [] - db.session.commit() +@contextmanager +def _public_role_like(role_name: str) -> Iterator[None]: + """Temporarily sync Public to a role and restore its exact prior state.""" + original_value = app.config["PUBLIC_ROLE_LIKE"] + original_permission_ids = [ + permission.id for permission in security_manager.get_public_role().permissions + ] + try: + app.config["PUBLIC_ROLE_LIKE"] = role_name + security_manager.sync_role_definitions() + yield + finally: + app.config["PUBLIC_ROLE_LIKE"] = original_value + # Tests and role syncing can commit or replace the session's ORM objects. + security_manager.get_public_role().permissions = ( Review Comment: Confirmed fixed, and I like this better than what I suggested. The three shared fixtures are back to master's teardown byte for byte, and the honest restore now lives in `public_role_like_gamma_scoped`, which only your six methods request, so the permission baseline the anonymous-user tests rely on is untouched. The four-file command (`dashboard_tests.py dashboards/api_tests.py users/api_tests.py themes/test_theme_api_permissions.py`) goes from `5 failed, 289 passed` to `294 passed` on a fresh sqlite DB. I also checked that the wipe was not just renamed: `Public` holds 90 permissions after `superset init`, still 90 after a `public_role_like_gamma_scoped` teardown, and 0 after the shared fixtures, exactly as on master. `users/api_tests.py` alone still fails its 4 deprivileged cases, which is master's own order dependency and not yours. The check that convinced me the grant is doing real work rather than sitting there: with `public_role_like_gamma_scoped` stubbed to grant nothing, `dashboard_tests.py` followed by `dashboards/api_tests.py` gives back exactly the original `12 failed, 255 passed`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
