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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 30, 2024
Merged

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

merged 1 commit into from
Nov 30, 2024

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

lmignon
lmignon suggested changes Nov 7, 2024
View reviewed changes
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

@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 ?

Thanks. I will continue it

@legalsylvain
Copy link
Contributor

legalsylvain commented Nov 7, 2024 •
edited
Loading

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 !

lmignon
lmignon suggested changes Nov 8, 2024
View reviewed changes
base_product_mass_addition/tests/test_product_mass_addition.py Outdated
@classmethod
def tearDownClass(cls):
cls.loader.restore_registry()
return super().tearDownClass()
Copy link
Contributor

@lmignon lmignon Nov 8, 2024 •
edited
Loading

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

lmignon
lmignon suggested changes Nov 8, 2024
View reviewed changes
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
@chaule97 chaule97 requested a review from lmignon November 11, 2024 02:40
lmignon
lmignon suggested changes Nov 12, 2024
View reviewed changes
product_abc_classification_sale_stock/tests/test_abc_classification_profile.py Outdated
@@ -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 requested a review from lmignon November 12, 2024 10:16
@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
lmignon
lmignon approved these changes Nov 13, 2024
View reviewed changes
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). 🤖

@dreispt
Copy link
Member

dreispt commented Nov 30, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1763-by-dreispt-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 30, 2024
@OCA-git-bot
Merge PR #1763 into 16.0
881a527 
Signed-off-by dreispt
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 16.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 16.0-ocabot-merge-pr-1763-by-dreispt-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit dbc073f into OCA:16.0 Nov 30, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at dbc073f. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousseldenis rousseldenis rousseldenis approved these changes

@legalsylvain legalsylvain legalsylvain approved these changes

@lmignon lmignon lmignon approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chaule97 @OCA-git-bot @rousseldenis @legalsylvain @dreispt @lmignon