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] tests: fix remaining patcher in python 3.10.12 #137215

Closed
wants to merge 2 commits into from

Conversation

Xavier-Do
Copy link
Contributor

@Xavier-Do Xavier-Do commented Oct 2, 2023

While deploying runbot 200-204 an error started to occur on TestEquipement

https://runbot.odoo.com/runbot/build/51753527
And also in the lunch single module
https://runbot.odoo.com/runbot/build/51708103

A patcher was remaining active at the end of TestEquipment, disabling it...

This is caused by a freeze_time on the previous executed class, TestAlarm (actually any class inheriting from odoo.addons.lunch.tests.common.TestsCommon)

It looks like the issue appeared between 3.10.6 and 3.10.12 (maybe because of python/cpython#99699): _class_cleanups is now on the TestCase. This is still unsure since we have a custom version of TestCase, could be a strange interaction.

Could be related to spulec/freezegun#485

Switching the class decorator inside the setupClass fixes the issue

Freezetime code managing decorator on TestCase as a reference:

    def decorate_class(self, klass):
        if issubclass(klass, unittest.TestCase):
            # If it's a TestCase, we assume you want to freeze the time for the
            # tests, from setUpClass to tearDownClass

            # Use getattr as in Python 2.6 they are optional
            orig_setUpClass = getattr(klass, 'setUpClass', None)
            orig_tearDownClass = getattr(klass, 'tearDownClass', None)

            # noinspection PyDecorator
            @classmethod
            def setUpClass(cls):
                self.start()
                if orig_setUpClass is not None:
                    orig_setUpClass()

            # noinspection PyDecorator
            @classmethod
            def tearDownClass(cls):
                if orig_tearDownClass is not None:
                    orig_tearDownClass()
                self.stop()

            klass.setUpClass = setUpClass
            klass.tearDownClass = tearDownClass

            return klass

@robodoo
Copy link
Contributor

robodoo commented Oct 2, 2023

Pull request status dashboard

Using freezetime to decorate a TestCase can have strange effects.

It looks like an override but monkeypatching the
setUpClass/tearDownClass. It started breaking between python 3.10.6 and
3.10.12. See pr message for more details.
@Xavier-Do Xavier-Do changed the title [IMP] tests: improve logging of active patcher [IMP] tests: fix remaining patcher with python 3.10.12 Oct 2, 2023
@Xavier-Do Xavier-Do changed the title [IMP] tests: fix remaining patcher with python 3.10.12 [IMP] tests: fix remaining patcher in python 3.10.12 Oct 2, 2023
@Xavier-Do Xavier-Do changed the title [IMP] tests: fix remaining patcher in python 3.10.12 [FIX] tests: fix remaining patcher in python 3.10.12 Oct 2, 2023
@C3POdoo C3POdoo added the RD research & development, internal work label Oct 2, 2023
@C3POdoo C3POdoo requested review from a team October 2, 2023 13:25
@xmo-odoo
Copy link
Collaborator

xmo-odoo commented Oct 2, 2023

FWIW there's also freeze_time class decorators in mrp (to... today???), account_peppol, purchase_stock, sale, and a bunch of others, which I'd assume are also affected.

edit: semgrep config for catching all them pokemans:

rules:
- id: search
  message: found hits
  severity: INFO
  languages: [python]
  patterns:
  - pattern: |
      @freezegun.freeze_time(...)
      class $C:
        ...

@d-fence
Copy link
Contributor

d-fence commented Oct 3, 2023

robodoo r+
The other cases can be handled in other Pr as they don't seem to break the tests.

@Xavier-Do
Copy link
Contributor Author

Xavier-Do commented Oct 3, 2023

FWIW there's also freeze_time class decorators in mrp (to... today???), account_peppol, purchase_stock, sale, and a bunch of others, which I'd assume are also affected.

Yes good catch, it may be a good idea to forbid that since it can be bug prone
Anyway, I think that the issue here is only when the freezetime is on an inherited class this is why we don't have to fix every places.

edit: semgrep config for catching all them pokemans:

rules:
- id: search
  message: found hits
  severity: INFO
  languages: [python]
  patterns:
  - pattern: |
      @freezegun.freeze_time(...)
      class $C:
        ...

Did it with a regexe @freeze_time.+\nclass, really need to dive into semgrep one day :)

@robodoo
Copy link
Contributor

robodoo commented Oct 3, 2023

@Xavier-Do @d-fence because this PR has multiple commits, I need to know how to merge it:

  • merge to merge directly, using the PR as merge commit message
  • rebase-merge to rebase and merge, using the PR as merge commit message
  • rebase-ff to rebase and fast-forward

@Xavier-Do
Copy link
Contributor Author

@robodoo rebase-ff

@robodoo
Copy link
Contributor

robodoo commented Oct 3, 2023

Merge method set to rebase and fast-forward.

robodoo pushed a commit that referenced this pull request Oct 3, 2023
robodoo pushed a commit that referenced this pull request Oct 3, 2023
Using freezetime to decorate a TestCase can have strange effects.

It looks like an override but monkeypatching the
setUpClass/tearDownClass. It started breaking between python 3.10.6 and
3.10.12. See pr message for more details.

closes #137215

Signed-off-by: Christophe Monniez (moc) <moc@odoo.com>
@robodoo robodoo temporarily deployed to merge October 3, 2023 08:42 Inactive
@robodoo robodoo closed this Oct 3, 2023
@robodoo robodoo added the 16.5 label Oct 3, 2023
@fw-bot fw-bot deleted the master-fix-runbot200-xdo branch October 17, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
16.5 RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants