[16.0][MIG] sale_timesheet_line_exclude

[CRogos]

Migration using oca-port tool

[CRogos]

@rafaelbn are the codecov steps working properly?

[rafaelbn]

/ocabor migration sale_timesheet_line_exclude

[nimarosa]

/ocabot migration sale_timesheet_line_exclude

[MohamedOsman7]

Functional changes & code LGTM

[Yadier-Tecnativa]

LGTM

[OCA-git-bot]

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). 🤖

[OCA-git-bot]

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). 🤖

[pedrobaeza]

Why removing these tests?

And please use local imports:

from . import ...

[CRogos]

I've added test_project_profitability, but I am unsure what you mean with use local imports.

Shell I create a new test file for the derived sale_timesheet tests?

test_reinvoice.py

from odoo.addons.sale_timesheet.tests.test_reinvoice import TestReInvoice

@tagged('-at_install', 'post_install')
class TestLineExcludeReInvoice(TestReInvoice):

[CRogos]

@pedrobaeza could you give me a hint how to improve the imports?

[pedrobaeza]

I was talking about __init__.py file. Inside test files, if they belong to other modules, you should use absolute imports.

[CRogos]

yes these test belong to a module this module depends on. But I am not sure how to use absolute imports.

[CRogos]

I think the idea of adding the base tests was to check, that everything is working as before, when the line exclude option is not checked. I think it is good practice to test the new option once checked and once not checked (here by executing the original tests).

We just had a similar case with the test here, but it was solved by deriving the test class from the base class: (https://github.com/OCA/sale-workflow/blob/16.0/sale_order_revision/tests/test_sale_order_revision.py)

from odoo.addons.base_revision.tests import test_base_revision
class TestSaleOrderRevision(test_base_revision.TestBaseRevision):

So I see the following options:

• leave it as it is
• remove the base tests
• add a derived test class for each base test class

[CRogos]

@pedrobaeza could you have a look on the test changes of my last commit? Is this the correct way to implement? If the changes are fine, I will squash the last two commits.

[pedrobaeza]

Do you need to override setUpClass method in inherited classes for this to work?

I think the idea of adding the base tests was to check, that everything is working as before, when the line exclude option is not checked. I think it is good practice to test the new option once checked and once not checked (here by executing the original tests).

Although the idea is not bad, this has a lot of cons:

• You don't control the upstream tests and the rest of the modules in the repo, so they can break for whatever other reason that it's out of scope of your module (for example, another module in the repo breaking such tests).
• They are very slow, as they are checking a lot of things that you don't need to recheck.
• Now with your approach, there's no problem in identifying the errors in the logs. With the previous one, there was.

Due to this, my usual approach when I want to test that an option that can be activated is not interfering with standard flows is to include explicit tests on the module test suite, focusing on the specific features that the option is touching.

This may reduce the scope and may have a side effect not covered, but it's not usually needed when the feature is very clear like this one. If a side effect arises later, a regression tests is the tool to avoid that side effect on the future.

[CRogos]

A class without content causes pre-commit to fail.

	Traceback (most recent call last):
	  File "/home/vscode/.cache/pre-commit/repokwtra7qi/py_env-python3/lib/python3.11/site-packages/pre_commit_hooks/debug_statement_hook.py", line 56, in check_file
	    ast_obj = ast.parse(f.read(), filename=filename)
	              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	  File "/usr/local/lib/python3.11/ast.py", line 50, in parse
	    return compile(source, filename, mode, flags,
	           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	  File "sale_timesheet_line_exclude/tests/test_project_billing.py", line 4
	    class TestProjectBillingLineExclude(TestProjectBilling):
	                                                            ^
	IndentationError: expected an indented block after class definition on line 4

Due to this, my usual approach when I want to test that an option that can be activated is not interfering with standard flows is to include explicit tests on the module test suite, focusing on the specific features that the option is touching.

Here you mean copy-past the test? In this case I would rather execute more unnecessary tests with the risk that the original test will break, than applying each change to the copied test.

I think removing the tests should be OK, as there is already a test checking that exclude_from_sale_order False is added to the invoice:

@pedrobaeza Do you agree?

[CRogos]

@pedrobaeza tests removed, can you merge the PR?

[pedrobaeza]

Here you mean copy-past the test? In this case I would rather execute more unnecessary tests with the risk that the original test will break, than applying each change to the copied test.

Not per se, but to test the exactly thing we want to check. That thing can be copied/pasted if it's being tested upstream, but without all the unneeded noise.

I think removing the tests should be OK, as there is already a test checking that exclude_from_sale_order False is added to the invoice:

OK for me too.

[pedrobaeza]

/ocabot merge nobump

[OCA-git-bot]

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-569-by-pedrobaeza-bump-nobump, awaiting test results.

[OCA-git-bot]

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