-
Notifications
You must be signed in to change notification settings - Fork 5
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
T258401: Stop deleting important objects found in commits #128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should restrict the detection to the top-level acl_users
. I think it should still be possible to remove any acl_users
that resides somewhere else (I might be convinced otherwise).
I thought exactly the same thing when reading the PR. The top-level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One typo and one question, which the tests answers in the positive… I'm just having difficulty understanding the deletion process fully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
self.run('playback', '/') | ||
assert 'acl_users' not in self.app.some_module.objectIds() | ||
|
||
def test_keep_acl_norecurse(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tipp: If you have two very similar tests that only differ in a few lines, you can write one test with two variants using pytest.parametrize
.
perfact/zodbsync/zodbsync.py
Outdated
del_ids = [ | ||
a for a in srv_contents | ||
if a not in contents and | ||
(path != '/acl_users' and a != 'acl_users') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why doesn't this prevent the deletion of /some_module/acl_users
in recursive mode? I'm a bit confused...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does prevent it, but the test does not notice because of some transaction shenanigans. I stepped with the debugger into the playback
and noticed that both fs_contents
and srv_contents
were empty, so nothing was deleted. Not sure why the first check for the presence of acl_users
in the tests does not fail though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To explain: If we simply manipulate something with directly accessing self.app
, this automatically opens a transaction. When the next self.run(...)
is executed, this usually also starts a transaction, aborting the already open transaction. Therefore the creation of /some_module
and /some_module/acl_users
was never commited, but visible in the first check (since it was still the same transaction), but no longer visible once the playback
started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess playback
explicitly starts a new transaction (to be able to roll it back later) while record
does not (because it does not expect to change anything inside the Data.FS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, fixed the remaining problem.
No description provided.