-
-
Notifications
You must be signed in to change notification settings - Fork 530
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
[15.0][IMP] subscription_oca: Tests, refactoring #991
[15.0][IMP] subscription_oca: Tests, refactoring #991
Conversation
ee35f1c
to
2996dd0
Compare
@carlos-domatix what do you think about tests? @simahawk implementation of #971 (review) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the effort :)
cls.product_1 = cls.env.ref("product.product_product_1") | ||
cls.product_1.taxes_id = [(6, 0, cls.tax_10pc_incl.ids)] | ||
cls.product_2 = cls.env.ref("product.product_product_2") | ||
cls.country_id = cls.env["res.country"].search([], limit=1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cls.country_id = cls.env["res.country"].search([], limit=1) | |
cls.country = cls.env["res.country"].search([], limit=1) |
to do for all vars that are records and not IDs
} | ||
) | ||
|
||
def test_all_subscription_oca(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any major reason to have just one biiiig test?
Still better than nothing of course :)
If you don't have time, add a TODO for splitting it
self.assertIsInstance(move_res, dict) | ||
|
||
# sale.subscription | ||
self.sub_id.cron_subscription_management() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should go to one or more cron specific tests
# sale.subscription.stage | ||
self.stage_id._check_lot_product() # should not raise | ||
|
||
# sale.subscription.template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testing a template? :)
b8c4ca9
to
7c003ce
Compare
@simahawk Thanks for the review. Please have a look again. |
Thanks a lot for the effort. I see it well but it has 70% coverage https://app.codecov.io/gh/OCA/contract/tree/15.0-subscription_oca-tests/, I don't know if it is enough for the repo, but at least it is a good base. |
/ocabot merge patch |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 7af1620. Thanks a lot for contributing to OCA. ❤️ |
No description provided.