[15.0][IMP] queue: test performance improvement #567
Conversation
- Create setUpClass() method if necessary - Switch to setUpClass (if needed) for avoiding repeat the same setup for each test. - Include context keys for avoiding mail operations overhead.
|
Hi @guewen, |
| super().setUp() | ||
| self.recordset = mock.MagicMock(name="recordset") | ||
| @classmethod | ||
| def setUpClass(cls): |
There was a problem hiding this comment.
IMO this change is useless. This class does not inherit from an odoo class and does not trigger any odoo functionality
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree 👍 Please, @josep-tecnativa :)
There was a problem hiding this comment.
Well, maybe in terms of "consistency" to not wondering if put it or not.
There was a problem hiding this comment.
The thing here is that this class test inherits directly from unittest so it's quite useless to set that
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@josep-tecnativa switch to BaseCase or TransactionCase, please.
|
This PR has the |
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
bot
added
the
stale
CC @Tecnativa TT45458