Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix performance issue #11451

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix performance issue #11451

wants to merge 1 commit into from

Conversation

sbrunner
Copy link
Member

@sbrunner sbrunner commented Oct 9, 2024

@sbrunner sbrunner force-pushed the preformance branch 6 times, most recently from b4b6606 to 254e54f Compare October 15, 2024 09:09
@sbrunner sbrunner force-pushed the preformance branch 2 times, most recently from 9faa3ec to 7621c7f Compare October 17, 2024 14:31
@sbrunner sbrunner marked this pull request as ready for review October 17, 2024 14:31
ger-benjamin
ger-benjamin reviewed Oct 23, 2024
View reviewed changes
Copy link
Member

@ger-benjamin ger-benjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhh I need @arnaud-morvan here please.

geoportal/tests/functional/__init__.py Outdated

import c2cgeoportal_geoportal
import c2cgeoportal_geoportal.lib
from c2cgeoportal_commons import models
from c2cgeoportal_geoportal.lib import caching

if TYPE_CHECKING:
from c2cgeoportal_commons.models import main, static
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waouw... is there no better way to achieve this ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only way to do:
It's needed at the top for mypy.
to the running application, it will break the application because it's not yet initialized.

arnaud-morvan
arnaud-morvan reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@arnaud-morvan arnaud-morvan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strategy here seems not clear to me.

We use multiple dbsession fixtures, sometimes with transaction, sometimes not, sometimes with commit inside some function.

IMHO we should have two dbsession fixture, one which is supposed to be committed, one which should not. And if possible none based on global models.DBSession.

geoportal/tests/functional/__init__.py Outdated
models.DBSession.query(Shorturl).delete()
models.DBSession.query(OAuth2Client).delete()
transaction.commit()
with models.DBSession() as session:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we should completely avoid using the global DBSession object and use a fixture instead, but I may miss something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to convert test_theme then this is part of code that I want to make minimal changes...

geoportal/tests/functional/__init__.py Outdated
from c2cgeoportal_commons.models.main import OGCServer

transaction.commit()
ogcserver = OGCServer(name="__test_ogc_server")
session.flush()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should flush at end of changes instead of before.
It would be really easier to debug.
Error would raise in function which made a mistake.

Also note that here pretty sure next SELECT will raise a flush.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in #11460

geoportal/tests/functional/conftest.py Outdated
@@ -46,23 +56,51 @@ def settings():


@pytest.fixture
def dbsession(settings):
def dbsession_old(settings: dict[str, Any]) -> Session:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep it, we could find a better name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal: dbsession_old_version?

@arnaud-morvan
Copy link
Member

The perfomance issue fix LGTM

sbrunner
sbrunner commented Nov 5, 2024
View reviewed changes
Copy link
Member Author

@sbrunner sbrunner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strategy here seems not clear to me.

We use multiple dbsession fixtures, sometimes with transaction, sometimes not, sometimes with commit inside some function.

The idea is to have the test_theme test clean as it should be and to the minimal changes for the other tests to make them work.

IMHO we should have two dbsession fixture, one which is supposed to be committed, one which should not. And if possible none based on global models.DBSession.

Effectively, we have many tests that not relay clear and require bad session...

The goal is to do one step in the right direction.

geoportal/tests/functional/__init__.py Outdated
models.DBSession.query(Shorturl).delete()
models.DBSession.query(OAuth2Client).delete()
transaction.commit()
with models.DBSession() as session:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to convert test_theme then this is part of code that I want to make minimal changes...

geoportal/tests/functional/conftest.py Outdated
@@ -46,23 +56,51 @@ def settings():


@pytest.fixture
def dbsession(settings):
def dbsession_old(settings: dict[str, Any]) -> Session:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal: dbsession_old_version?

geoportal/tests/functional/__init__.py Outdated

import c2cgeoportal_geoportal
import c2cgeoportal_geoportal.lib
from c2cgeoportal_commons import models
from c2cgeoportal_geoportal.lib import caching

if TYPE_CHECKING:
from c2cgeoportal_commons.models import main, static
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only way to do:
It's needed at the top for mypy.
to the running application, it will break the application because it's not yet initialized.

geoportal/tests/functional/__init__.py Outdated
from c2cgeoportal_commons.models.main import OGCServer

transaction.commit()
ogcserver = OGCServer(name="__test_ogc_server")
session.flush()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in #11460

@arnaud-morvan
Copy link
Member

How about merging only the performance fix here, and discuss about tests refactoring in #11460

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnaud-morvan arnaud-morvan arnaud-morvan left review comments

@ger-benjamin ger-benjamin ger-benjamin left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sbrunner @arnaud-morvan @ger-benjamin