fix persistency bug in the Python version #147
Conversation
|
Marius Gedminas wrote at 2020-11-6 16:02 -0800:
@mgedmin requested changes on this pull request.
...
You're missing the `<` `>` around the URL.
Thank you for this catch! Fixed.
|
|
Marius Gedminas wrote at 2020-11-6 16:00 -0800:
@mgedmin approved this pull request.
...
Do you think it would be hard to write a regression test for this case?
The difficult part comes from the fact that only the Python implementation
is affected. Otherwise, a test would be simple:
construct a tree with 2 buckets; empty the second bucket;
commit (i.e. write to ZODB); reload; check.
Even when the C implementation is available, I can access
the Python implementation (via class names with `Py` suffix).
However, some magic seems to cause a class name change
which might affect storing and loading the tree
(I suppose that at least the C implementation classes are used
on reloading).
I can deactivate the C implmentation via the envvar `PURE_PYTHON`
(that's what I did in my manual test). However, the envvar
affects only the initial `BTrees` import. Therefore, this approach
is difficult in part of a test suite (where `BTrees` might
already have been imported for preceding tests).
I have seen in `tox.ini` that there are `*-pure` envs
which apparently set the envvar `PURE_PYTHON`.
But, the test above would require both `w_zodb` as well as
`-pure` and I have not seen a hint that they are available together.
|
|
I don't think it would be good to target the pure-python implementation specifically; it'd be better to have a generic test for this case that gets run against both C and Python implementations. We don't need to have a single test suite run cover all the implementations. We have multiple CI jobs for a reason, and some of them run with PURE_PYTHON=1. If we're missing a job with PURE_PYTHON and w_zodb in Travis CI, let's add one! I don't particularly care about tox.ini, unless it would help writing that test to have an additional toxenv, in which case go right ahead! |
I have added a test. When I run the test in isolation ( |
|
I have no time to lookup the exact syntax, but my gut feeling tells me this should work: If you want to look it up yourself, search for "tox factors". |
|
Jürgen Gmach wrote at 2020-11-9 01:57 -0800:
I have no time to lookup the exact syntax, but my gut feeling tells me this should work:
```
[testenv:{py35,w_zodb}-pure]
setenv =
PURE_PYTHON=1
```
If you want to look it up yourself, search for "tox factors".
The current `tox.ini` contains:
```
[testenv]
...
setenv =
...
pure: PURE_PYTHON=1
```
As I understand the documentation this should work but apparently does not.
I am currently searching a way to convince `tox` to show the
environ variables it is using to executes the commands.
Even a large number of `-v` options does not do this.
|
…`w_zodb` -- copy them explicitly
|
The problem was not a missing I assumed that the I made several trials to cover both |
| def tearDown(self): | ||
| if self.db is not None: | ||
| self.db.close() | ||
| del self.db |
There was a problem hiding this comment.
Why this rather than, say, self.db = None?
| @@ -3,7 +3,7 @@ envlist = | |||
| # Jython support pending 2.7 support, due 2012-07-15 or so. See: | |||
| # http://fwierzbicki.blogspot.com/2012/03/adconion-to-fund-jython-27.html | |||
| # py27,jython,pypy,coverage,docs | |||
| py27,py27-pure,py35,py35-pure,py36,py37,py38,pypy,pypy3,w_zodb,coverage,docs | |||
| py27,py27-pure,py35,py35-pure,py36,py37,py38,pypy,pypy3,w_zodb,w_zodb-pure,coverage,docs | |||
There was a problem hiding this comment.
Do we have a .travis.yml job corresponding to this?
There was a problem hiding this comment.
Yes, AFAICT all of our Travis jobs include ZODB.
|
Marius Gedminas wrote at 2020-11-9 07:41 -0800:
@mgedmin approved this pull request.
lgtm
> def tearDown(self):
if self.db is not None:
self.db.close()
+ del self.db
Why this rather than, say, `self.db = None`?
I wanted to restore the initial state (where `db` is defined
at class level). Of course `self.db = None` would have almost the same
effect.
|
|
Marius Gedminas wrote at 2020-11-9 07:42 -0800:
@mgedmin commented on this pull request.
> @@ -3,7 +3,7 @@ envlist =
# Jython support pending 2.7 support, due 2012-07-15 or so. See:
# http://fwierzbicki.blogspot.com/2012/03/adconion-to-fund-jython-27.html
# py27,jython,pypy,coverage,docs
- py27,py27-pure,py35,py35-pure,py36,py37,py38,pypy,pypy3,w_zodb,coverage,docs
+ py27,py27-pure,py35,py35-pure,py36,py37,py38,pypy,pypy3,w_zodb,w_zodb-pure,coverage,docs
Do we have a .travis.yml job corresponding to this?
I have still very few knowledge about travis
(and made my first steps with `tox`).
|
Ah! I didn't realize that was the case
and would maybe avoid a possible bug in the future if somebody moves the class attribute into |
|
Marius Gedminas wrote at 2020-11-9 13:51 -0800:
...
and would maybe avoid a possible bug in the future if somebody moves the class attribute into `__init__` and doesn't realize they need to also update this code.
This is a `unittest.TestCase` class. Its `__init__` is (mostly) private
(and called for each individual test). Thus, there is no danger.
In fact, I added the `del` (only) to help me interactively debug why my test
(initially) did not reveal the problem. It is not important for
the real test suite (you can delete it if you like).
As to the problem: the persistency loophole is very small:
it affects only the case that a transaction deletes the last
item in a bucket without having previously deleted anything in this
bucket. My initial test had emptied the whole bucket in the same
transaction and this did not reproduce the problem.
|
I should stop reviewing code when I'm tired. I didn't even notice that! I retract my comments on this. |
fixes #118