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.

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

Restore possibility to change keep_session param. #40

Merged
merged 3 commits into from Feb 17, 2020
Merged

Conversation

icemac
Copy link
Member

@icemac icemac commented Nov 27, 2019

Before version 1.2 it was possible to change the value of keep_session for certain tests.
We used keep_session=True to ease unittests which are not forced to re-fetch objects from database after a commit. On the other hand selenium tests should behave more like an actual web server which does not keep the session.
So we switched to keep_session=False for some tests with code like this:
https://github.com/gocept/risclog.sqlalchemy/blob/44fe2494337b41e0bc3d5602ebbd7493918d841d/src/risclog/sqlalchemy/fixtures.py#L6

This is no longer possible since zope.sqlalchemy 1.2.
With this commit the ability to access the ZopeTransactionEvents instance is restored.

Before version 1.2 it was possible to change the value of `keep_session` for certain tests.
We used `keep_session=True` to ease unittests which are not forced to re-fetch objects from database after a commit. On the other hand selenium tests should behave more like an actual web server which does not keep the session.
So we switched to `keep_session=False` for some tests with code like this:
https://github.com/gocept/risclog.sqlalchemy/blob/44fe2494337b41e0bc3d5602ebbd7493918d841d/src/risclog/sqlalchemy/fixtures.py#L6

This is no longer possible since zope.sqlalchemy 1.2.
With this commit the ability to access the `ZopeTransactionEvents` instance is restored.
@icemac icemac added the bug label Nov 27, 2019
@icemac icemac self-assigned this Nov 27, 2019
@icemac icemac requested a review from mgedmin November 27, 2019 17:28
@icemac
Copy link
Member Author

icemac commented Nov 27, 2019

Could this be a possible solution where I should invest to fix the tests or does anyone think this is a bad idea.

Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

LGTM, but please document the return value in the docstring

@icemac icemac merged commit ca3a094 into master Feb 17, 2020
@icemac icemac deleted the icemac-patch-1 branch February 17, 2020 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants