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

[16.0][FIX] add addClassCleanup in testcase #1763

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

chaule97
Copy link
Contributor

@chaule97 chaule97 commented Nov 7, 2024

The test class in the product_abc_classification_sale_stock module has the same name as a test class in product_abc_classification, which is causing an error in GitHub CI in #1561, #1683

@OCA-git-bot
Copy link
Contributor

Hi @lmarion-source, @lmignon, @rousseldenis,
some modules you are maintaining are being modified, check this out!

@rousseldenis
Copy link
Contributor

The test class in the product_abc_classification_sale_stock module has the same name as a test class in product_abc_classification, which is causing an error in GitHub CI in #1561, #1683

@chaule97 I agreed on name change because it is duplicated from base module.

But the errors you point to are not related to that.

Please highlight the real problem you want to solve

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution but the name of a test class should never cause problem when running tests... IMO the diagnostic is not right. Moreover when you change the name of a class into a module named as the call, the module must also be renamed....

@rousseldenis
Copy link
Contributor

@chaule97 https://github.com/OCA/product-attribute/blob/16.0/base_product_mass_addition/tests/test_product_mass_addition.py#L21

This is the real error.

Missing a teardown(). See https://github.com/OCA/odoo-test-helper/blob/master/README.rst

You do the fix ?

@rousseldenis
Copy link
Contributor

Or @legalsylvain ?

@chaule97
Copy link
Contributor Author

chaule97 commented Nov 7, 2024

@legalsylvain
Copy link
Contributor

legalsylvain commented Nov 7, 2024

Feel free to add the teardown in this PR.

@chaule97
Copy link
Contributor Author

chaule97 commented Nov 7, 2024

Thanks everyone, I have added teardown

@legalsylvain
Copy link
Contributor

thanks @chaule97 !

@classmethod
def tearDownClass(cls):
cls.loader.restore_registry()
return super().tearDownClass()
Copy link
Contributor

@lmignon lmignon Nov 8, 2024

Choose a reason for hiding this comment

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

It's better to register the call to the restore_registry method in the list of cleanup methods to execute when all the tests into the class are executed just after the fake registry is backed up

cls.loader.backup_registry()
cls.addClassCleanup(cls.loader.restore_registry)

In this way, even if an exception occurs into your setup, the registry will be cleaned up

Copy link
Contributor

Choose a reason for hiding this comment

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

@lmignon Isn't it supported by test class ?

If not, odoo-test-helper documentation should be adapted too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmignon I have changed it

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

see my last comment

@chaule97 chaule97 force-pushed the fix-product_abc_classification_sale_stock branch from a314805 to 8ca0c24 Compare November 11, 2024 02:35
@@ -6,7 +6,7 @@
from odoo.tests.common import TransactionCase


class TestABCClassificationProfile(TransactionCase):
class TestABCClassificationProfileSaleStock(TransactionCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

@chaule97 This change is useless but if you change the classname you must change the filename.

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 have renamed

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 and we must avoid useless changes. Is-it really a problem for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I have removed this commit. I remember Odoo 17 have issue when it have duplicate testcase class name. I think that it was fixed in version 18.0

OCA/pos#1232

@chaule97 chaule97 force-pushed the fix-product_abc_classification_sale_stock branch from 8ca0c24 to 62d38ed Compare November 12, 2024 10:06
@chaule97 chaule97 force-pushed the fix-product_abc_classification_sale_stock branch from 62d38ed to 5baf68a Compare November 13, 2024 02:41
@chaule97 chaule97 changed the title [16.0][FIX] product_abc_classification_sale_stock: fix duplicate test class names [16.0][FIX] add addClassCleanup in testcase Nov 13, 2024
Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Thank you for this fix!

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

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.

5 participants