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

[15.0][IMP] queue: test performance improvement #567

Closed
wants to merge 1 commit into from
Closed

[15.0][IMP] queue: test performance improvement #567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions queue_job/tests/test_delayable.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,20 @@


class TestDelayable(unittest.TestCase):
def setUp(self):
super().setUp()
self.recordset = mock.MagicMock(name="recordset")
@classmethod
def setUpClass(cls):
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this change is useless. This class does not inherit from an odoo class and does not trigger any odoo functionality

Copy link
Member

Choose a reason for hiding this comment

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

Well, it avoids redeclaring the case for every test. Indeed not much gain here anyway and the context thing would be useless here, that's true

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a mocked object... changes nothing in terms of perf.
Anyway, I'd keep the setUpClass but I'd drop the ctx stuff because it gives the false idea that is needed for whatever reason.

Copy link
Member

Choose a reason for hiding this comment

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

I agree 👍 Please, @josep-tecnativa :)

Copy link
Member

Choose a reason for hiding this comment

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

Well, maybe in terms of "consistency" to not wondering if put it or not.

Copy link
Member

Choose a reason for hiding this comment

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

The thing here is that this class test inherits directly from unittest so it's quite useless to set that

Copy link
Member

Choose a reason for hiding this comment

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

Yes, useless, but consistent to always read the same code everywhere in OCA (I wonder also why this is not using Odoo tests), and with minimum overhead. And the day this is switched to Odoo tests, you already have it.

Copy link
Contributor

@simahawk simahawk Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree because you should always keep the setup minimal.
However.... Now that I think of this twice... I fear that since the refactoring of the test suite was done (14, 15, 16) these tests are not auto-discovered anymore (just search for this test class in the build).
Hence, we should use BaseCase at least.

Copy link
Member

Choose a reason for hiding this comment

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

@josep-tecnativa switch to BaseCase or TransactionCase, please.

super().setUpClass()

Check warning on line 13 in queue_job/tests/test_delayable.py

View check run for this annotation

Codecov / codecov/patch

queue_job/tests/test_delayable.py#L13 

Added line #L13 was not covered by tests
# Remove this variable in v16 and put instead:
# from odoo.addons.base.tests.common import DISABLED_MAIL_CONTEXT
DISABLED_MAIL_CONTEXT = {

Check warning on line 16 in queue_job/tests/test_delayable.py

View check run for this annotation

Codecov / codecov/patch

queue_job/tests/test_delayable.py#L16 

Added line #L16 was not covered by tests
"tracking_disable": True,
"mail_create_nolog": True,
"mail_create_nosubscribe": True,
"mail_notrack": True,
"no_reset_password": True,
}
cls.env = cls.env(context=dict(cls.env.context, **DISABLED_MAIL_CONTEXT))
cls.recordset = mock.MagicMock(name="recordset")

Check warning on line 24 in queue_job/tests/test_delayable.py

View check run for this annotation

Codecov / codecov/patch

queue_job/tests/test_delayable.py#L23-L24 

Added lines #L23 - L24 were not covered by tests

def test_delayable_set(self):
dl = Delayable(self.recordset)
Expand Down
14 changes: 14 additions & 0 deletions queue_job/tests/test_json_field.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,20 @@


class TestJson(common.TransactionCase):
@classmethod
def setUpClass(cls):
super().setUpClass()
# Remove this variable in v16 and put instead:
# from odoo.addons.base.tests.common import DISABLED_MAIL_CONTEXT
DISABLED_MAIL_CONTEXT = {
"tracking_disable": True,
"mail_create_nolog": True,
"mail_create_nosubscribe": True,
"mail_notrack": True,
"no_reset_password": True,
}
cls.env = cls.env(context=dict(cls.env.context, **DISABLED_MAIL_CONTEXT))

def test_encoder_recordset(self):
demo_user = self.env.ref("base.user_demo")
context = demo_user.context_get()
Expand Down
19 changes: 15 additions & 4 deletions queue_job/tests/test_model_job_channel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,21 @@


class TestJobChannel(common.TransactionCase):
def setUp(self):
super().setUp()
self.Channel = self.env["queue.job.channel"]
self.root_channel = self.Channel.search([("name", "=", "root")])
@classmethod
def setUpClass(cls):
super().setUpClass()
# Remove this variable in v16 and put instead:
# from odoo.addons.base.tests.common import DISABLED_MAIL_CONTEXT
DISABLED_MAIL_CONTEXT = {
"tracking_disable": True,
"mail_create_nolog": True,
"mail_create_nosubscribe": True,
"mail_notrack": True,
"no_reset_password": True,
}
cls.env = cls.env(context=dict(cls.env.context, **DISABLED_MAIL_CONTEXT))
cls.Channel = cls.env["queue.job.channel"]
cls.root_channel = cls.Channel.search([("name", "=", "root")])

def test_channel_new(self):
channel = self.Channel.new()
Expand Down
14 changes: 14 additions & 0 deletions queue_job/tests/test_model_job_function.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,20 @@


class TestJobFunction(common.TransactionCase):
@classmethod
def setUpClass(cls):
super().setUpClass()
# Remove this variable in v16 and put instead:
# from odoo.addons.base.tests.common import DISABLED_MAIL_CONTEXT
DISABLED_MAIL_CONTEXT = {
"tracking_disable": True,
"mail_create_nolog": True,
"mail_create_nosubscribe": True,
"mail_notrack": True,
"no_reset_password": True,
}
cls.env = cls.env(context=dict(cls.env.context, **DISABLED_MAIL_CONTEXT))

def test_function_name_compute(self):
function = self.env["queue.job.function"].create(
{"model_id": self.env.ref("base.model_res_users").id, "method": "read"}
Expand Down
14 changes: 14 additions & 0 deletions queue_job/tests/test_queue_job_protected_write.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,20 @@


class TestJobWriteProtected(common.TransactionCase):
@classmethod
def setUpClass(cls):
super().setUpClass()
# Remove this variable in v16 and put instead:
# from odoo.addons.base.tests.common import DISABLED_MAIL_CONTEXT
DISABLED_MAIL_CONTEXT = {
"tracking_disable": True,
"mail_create_nolog": True,
"mail_create_nosubscribe": True,
"mail_notrack": True,
"no_reset_password": True,
}
cls.env = cls.env(context=dict(cls.env.context, **DISABLED_MAIL_CONTEXT))

def test_create_error(self):
with self.assertRaises(exceptions.AccessError):
self.env["queue.job"].create(
Expand Down
23 changes: 17 additions & 6 deletions queue_job/tests/test_wizards.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,28 @@


class TestWizards(common.TransactionCase):
def setUp(self):
super().setUp()
self.job = (
self.env["queue.job"]
@classmethod
def setUpClass(cls):
super().setUpClass()
# Remove this variable in v16 and put instead:
# from odoo.addons.base.tests.common import DISABLED_MAIL_CONTEXT
DISABLED_MAIL_CONTEXT = {
"tracking_disable": True,
"mail_create_nolog": True,
"mail_create_nosubscribe": True,
"mail_notrack": True,
"no_reset_password": True,
}
cls.env = cls.env(context=dict(cls.env.context, **DISABLED_MAIL_CONTEXT))
cls.job = (
cls.env["queue.job"]
.with_context(
_job_edit_sentinel=self.env["queue.job"].EDIT_SENTINEL,
_job_edit_sentinel=cls.env["queue.job"].EDIT_SENTINEL,
)
.create(
{
"uuid": "test",
"user_id": self.env.user.id,
"user_id": cls.env.user.id,
"state": "failed",
"model_name": "queue.job",
"method_name": "write",
Expand Down