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

[FIX] database_cleanup: Fix test #612

Merged
merged 9 commits into from
Nov 22, 2016

Conversation

moylop260
Copy link
Contributor

@moylop260 moylop260 commented Nov 17, 2016

For the merger: I @moylop260 authorize a squash and merge here in order to leave the commits comments like as history.

- Follow recommendation from odoo/odoo#13458 (comment)
- Fix blocking tables issues
- Delete trash information
# Release the delete of ir_module_module pending
self.env.cr.rollback()
cr2.execute(
"DELETE FROM ir_module_module WHERE id=%s", (module.id,))
Copy link
Member

Choose a reason for hiding this comment

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

if this is necessary, the test has failed. Or better: Then we have the problem that the commits the module needs to do shouldn't be committed. Maybe it's better to defuse cr.commit at the beginning of the test and reinstate it here?

Copy link
Contributor Author

@moylop260 moylop260 Nov 17, 2016

Choose a reason for hiding this comment

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

If you run 2 times this test, you will a contraint error of ir_module_module name unique.
It's because the process: self.env['ir.module.module'].create(...) and RegistryManager.registries[self.env.cr.dbname] = original_registry have a custom cursor with commit (different to current self.env.cr)

Then the test purge using self.env.cr and work fine because that cursor have a delete.
But all tests use rollback at final.
Then you will have a dirty database with a module called: database_cleanup_test

I mean, without this patch

  • cursor_x
BEGIN;
INSERT INTO ir_module_module
COMMIT;
END;
  • self.env.cr
BEGIN;
.... # many other transactions
DELETE FROM ir_module_module
ROLLBACK;
END;

Copy link
Member

Choose a reason for hiding this comment

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

I see. Well, it's not beautiful, but I don't think it's worth the time to get this cursor's commit patched away to by starting a patch fest of the involved methods. Ignore this comment then

@@ -87,11 +87,15 @@ def purge(self):
pass
except AttributeError:
pass
if line.name not in self.env:
Copy link
Member

Choose a reason for hiding this comment

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

do we need this now that the test runs at the end of everything? The problem is that for the actual use case of the module, this will happen very often, and reloading the registry for every unknown model will take very long. The alternative would be to check for the testing flag and do it only then if it's still necessary for the test

self.env['ir.model'].browse([row[0]])\
.with_context(**context_flags).unlink()
model = self.env['ir.model'].browse([row[0]])
if line.name in self.env:
Copy link
Member

Choose a reason for hiding this comment

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

this defeats the purpose of purging undefined modules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know but I didn't find the way to fix the issue (I revert this change to see the error from travis):

   ======================================================================
    ERROR: test_database_cleanup (openerp.addons.database_cleanup.tests.test_database_cleanup.TestDatabaseCleanup)
    Traceback (most recent call last):
    `   File "/root/build/OCA/server-tools/database_cleanup/tests/test_database_cleanup.py", line 58, in test_database_cleanup
    `     purge_models.purge_all()
    `   File "/.repo_requirements/odoo/openerp/api.py", line 248, in wrapper
    `     return new_api(self, *args, **kwargs)
     `   File "/root/build/OCA/server-tools/database_cleanup/models/purge_wizard.py", line 51, in purge_all
     `     self.mapped('purge_line_ids').purge()
     `   File "/.repo_requirements/odoo/openerp/api.py", line 248, in wrapper
     `     return new_api(self, *args, **kwargs)
     `   File "/root/build/OCA/server-tools/database_cleanup/models/purge_models.py", line 94, in purge
     `     model.with_context(**context_flags).unlink()
     `   File "/.repo_requirements/odoo/openerp/api.py", line 248, in wrapper
     `     return new_api(self, *args, **kwargs)
     `   File "/.repo_requirements/odoo/openerp/api.py", line 574, in new_api
     `     result = method(self._model, cr, uid, self.ids, *args, **old_kwargs)
     `   File "/.repo_requirements/odoo/openerp/addons/base/ir/ir_model.py", line 147, in unlink
     `     model.field_id._prepare_update()
     `   File "/.repo_requirements/odoo/openerp/api.py", line 248, in wrapper
     `     return new_api(self, *args, **kwargs)
     `   File "/.repo_requirements/odoo/openerp/addons/base/ir/ir_model.py", line 445, in _prepare_update
     `     model = self.env[record.model]
     `   File "/.repo_requirements/odoo/openerp/api.py", line 768, in __getitem__
     `     return self.registry[model_name]._browse(self, ())
     `   File "/.repo_requirements/odoo/openerp/modules/registry.py", line 84, in __getitem__
     `     return self.models[model_name]
     ` KeyError: u'x_database.cleanup.test.model'

@hbrunn hbrunn added this to the 9.0 milestone Nov 17, 2016
To show the error
======================================================================
ERROR: test_database_cleanup (openerp.addons.database_cleanup.tests.test_database_cleanup.TestDatabaseCleanup)
Traceback (most recent call last):
`   File "/root/build/OCA/server-tools/database_cleanup/tests/test_database_cleanup.py", line 58, in test_database_cleanup
`     purge_models.purge_all()
`   File "/.repo_requirements/odoo/openerp/api.py", line 248, in wrapper
`     return new_api(self, *args, **kwargs)
 `   File "/root/build/OCA/server-tools/database_cleanup/models/purge_wizard.py", line 51, in purge_all
 `     self.mapped('purge_line_ids').purge()
 `   File "/.repo_requirements/odoo/openerp/api.py", line 248, in wrapper
 `     return new_api(self, *args, **kwargs)
 `   File "/root/build/OCA/server-tools/database_cleanup/models/purge_models.py", line 94, in purge
 `     model.with_context(**context_flags).unlink()
 `   File "/.repo_requirements/odoo/openerp/api.py", line 248, in wrapper
 `     return new_api(self, *args, **kwargs)
 `   File "/.repo_requirements/odoo/openerp/api.py", line 574, in new_api
 `     result = method(self._model, cr, uid, self.ids, *args, **old_kwargs)
 `   File "/.repo_requirements/odoo/openerp/addons/base/ir/ir_model.py", line 147, in unlink
 `     model.field_id._prepare_update()
 `   File "/.repo_requirements/odoo/openerp/api.py", line 248, in wrapper
 `     return new_api(self, *args, **kwargs)
 `   File "/.repo_requirements/odoo/openerp/addons/base/ir/ir_model.py", line 445, in _prepare_update
 `     model = self.env[record.model]
 `   File "/.repo_requirements/odoo/openerp/api.py", line 768, in __getitem__
 `     return self.registry[model_name]._browse(self, ())
 `   File "/.repo_requirements/odoo/openerp/modules/registry.py", line 84, in __getitem__
 `     return self.models[model_name]
 ` KeyError: u'x_database.cleanup.test.model'
@moylop260 moylop260 force-pushed the 9.0-fixes-cleanup-moy branch from 0152b5e to a76e783 Compare November 17, 2016 15:39
@pedrobaeza
Copy link
Member

@moylop260, please rebase that I have fixed 2 small PEP8 problems.

self.env.cr.execute(
'insert into ir_attachment (name, res_model, res_id, type) values '
"('test attachment', 'database.cleanup.test.model', 42, 'binary')")
self.registry.models.pop('x_database.cleanup.test.model')
self.registry._pure_function_fields.pop(
'x_database.cleanup.test.model')
self.registry.setup_models(self.env.cr, partial=False)
Copy link
Member

Choose a reason for hiding this comment

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

right, doing this actually makes this a good model, so the next line fails because there's nothing to purge. Just pop the model from the registry immediately, then this goes well

….model'

Traceback (most recent call last):
`   File "~/build/OCA/server-tools/database_cleanup/tests/test_database_cleanup.py", line 65, in test_database_cleanup
`     purge_models.purge_all()
`   File "~/build/OCA/server-tools/database_cleanup/models/purge_wizard.py", line 51, in purge_all
`     self.mapped('purge_line_ids').purge()
`   File "~/build/OCA/server-tools/database_cleanup/models/purge_models.py", line 94, in purge
`     .with_context(**context_flags).unlink()
`   File "~/odoo/openerp/api.py", line 574, in new_api
`     result = method(self._model, cr, uid, self.ids, *args, **old_kwargs)
`   File "~/odoo/openerp/addons/base/ir/ir_model.py", line 147, in unlink
`     model.field_id._prepare_update()
`   File "~/odoo/openerp/addons/base/ir/ir_model.py", line 445, in _prepare_update
`     model = self.env[record.model]
`   File "~/odoo/openerp/api.py", line 768, in __getitem__
`     return self.registry[model_name]._browse(self, ())
`   File "~/odoo/openerp/modules/registry.py", line 84, in __getitem__
`     return self.models[model_name]
` KeyError: u'x_database.cleanup.test.model'
2016-11-18 12:53:24,269 11158 INFO moy42 openerp.addons.database_cleanup.tests.test_database_cleanup: Ran 1 test in 2.599s
FAILED
@moylop260
Copy link
Contributor Author

moylop260 commented Nov 18, 2016

@hbrunn
Thanks for feedback

IMHO this is a real issue from openerp/addons/base/ir/ir_model.py#L147
Inside model.field_id._prepare_update() odoo is using self.env[record.model] added from odoo/odoo#13978

But here we don't have a model in environment.
Then we have the key error by this reason.

@moylop260
Copy link
Contributor Author

moylop260 commented Nov 18, 2016

That line is not exists from 8.0 version then if I comment this one the test pass fine!

Then we should create a fix to odoo/odoo before.

It mean that database_cleanup it is not working for 9.0 for now to delete models.

@moylop260
Copy link
Contributor Author

We have false red builds by this issue.
Now is false green just for database_cleanup since the problem inject from odoo/odoo#13978

IMHO we should avoid the false red for other builds with this PR
What do you think?

@pedrobaeza pedrobaeza mentioned this pull request Nov 22, 2016
@sbidoul
Copy link
Member

sbidoul commented Nov 22, 2016

@hbrunn what's the status here? Is this good for merging to get the 9.0 branch green?

@pedrobaeza pedrobaeza merged commit ab51acc into OCA:9.0 Nov 22, 2016
@moylop260 moylop260 deleted the 9.0-fixes-cleanup-moy branch November 22, 2016 14:07
ecino pushed a commit to CompassionCH/server-tools that referenced this pull request Jul 10, 2017
hbrunn pushed a commit to hbrunn/server-tools that referenced this pull request Oct 3, 2017
hbrunn pushed a commit to hbrunn/server-tools that referenced this pull request Feb 13, 2018
Rad0van pushed a commit to Rad0van/server-tools that referenced this pull request Aug 15, 2020
StephaneMangin pushed a commit to StephaneMangin/server-tools that referenced this pull request Jan 12, 2021
Rad0van pushed a commit to Rad0van/server-tools that referenced this pull request Jan 12, 2021
leemannd pushed a commit to leemannd/server-tools that referenced this pull request Mar 19, 2021
leemannd pushed a commit to camptocamp/server-tools that referenced this pull request May 31, 2021
StephaneMangin pushed a commit to StephaneMangin/server-tools that referenced this pull request Sep 13, 2021
sebalix pushed a commit to sebalix/server-tools that referenced this pull request Sep 23, 2021
leemannd pushed a commit to camptocamp/server-tools that referenced this pull request Oct 18, 2021
leemannd pushed a commit to camptocamp/server-tools that referenced this pull request Oct 19, 2021
damdam-s pushed a commit to damdam-s/server-tools that referenced this pull request Dec 3, 2021
psugne pushed a commit to versada/server-tools that referenced this pull request Dec 23, 2021
StephaneMangin pushed a commit to StephaneMangin/server-tools that referenced this pull request Feb 1, 2022
damdam-s pushed a commit to damdam-s/server-tools that referenced this pull request Apr 14, 2022
sebalix pushed a commit to camptocamp/server-tools that referenced this pull request May 4, 2022
thomaspaulb pushed a commit to sunflowerit/server-tools that referenced this pull request Aug 17, 2022
JoelZilli pushed a commit to adhoc-dev/server-tools that referenced this pull request Nov 24, 2022
ajaniszewska-dev pushed a commit to ajaniszewska-dev/server-tools that referenced this pull request Jan 4, 2023
yankinmax pushed a commit to yankinmax/server-tools that referenced this pull request Feb 9, 2023
JasminSForgeFlow pushed a commit to ForgeFlow/server-tools that referenced this pull request Jul 28, 2023
letzdoo-js pushed a commit to letzdoo-js/server-tools that referenced this pull request Aug 25, 2023
alexeirivera87 pushed a commit to archeti-org/server-tools that referenced this pull request Sep 6, 2023
jjscarafia pushed a commit to adhoc-dev/server-tools that referenced this pull request Oct 10, 2023
hbrunn pushed a commit to hbrunn/server-tools that referenced this pull request Dec 23, 2023
augusto-weiss pushed a commit to adhoc-dev/server-tools that referenced this pull request Jan 10, 2024
PieterPaulussen pushed a commit to The-O-Team/server-tools that referenced this pull request Jul 18, 2024
nguyenminhchien pushed a commit to nguyenminhchien/server-tools that referenced this pull request Sep 27, 2024
nguyenminhchien pushed a commit to nguyenminhchien/server-tools that referenced this pull request Sep 27, 2024
nguyenminhchien pushed a commit to nguyenminhchien/server-tools that referenced this pull request Oct 10, 2024
nguyenminhchien pushed a commit to nguyenminhchien/server-tools that referenced this pull request Oct 21, 2024
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (10.0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants