[FIX] contract unittest test_contract.py

[mt-software-de]

This unittest needs the installation of l10n_us and l10n_generic_coa
otherwise there are no account.journal
the install of the two modules happens in the account module with a post_init_hook

[rousseldenis]

@mt-software-de What is your problem exactly ?

Here under the logs from the last 14.0 branch (coa module is well loaded before tests):

[mt-software-de]

@rousseldenis
I am installing the module with --test-tags=at_install/contract,post_install/contract
odoo-bin -i contract --test-tags=at_install/contract,post_install/contract

As you can see in my log
the contract module is loaded before l10n_generic_coa

So if i am installing it with --test-tags at_install
the test runs right after the contract module installation
By default every test is tagged as at_install and standard

[rousseldenis]

The good way is to initialize the db with all dependencies, then launch the tests

If you check logs, that's how it is done everywhere on OCA.

So, no need to change how specific modules tests are launched

[mt-software-de]

I have an testing environment where it is more like the odoo standard why
Tests where execute with --test-tags at_install and post_install
Not every test is executed as a post_install test like the you will do it

Like you can see for example in odoo core modules
odoo/addons/sale_stock/tests/test_sale_stock.py TestSaleStockOnly.test_no_automatic_assign
this will not work running it when the all the dependencies are initialized because then also the auto_install modules are loaded

[rousseldenis]

I have an testing environment where it is more like the odoo standard why
Tests where execute with --test-tags at_install and post_install
Not every test is executed as a post_install test like the you will do it

Like you can see for example in odoo core modules
odoo/addons/sale_stock/tests/test_sale_stock.py TestSaleStockOnly.test_no_automatic_assign
this will not work running it when the all the dependencies are initialized because then also the auto_install modules are loaded

The good flow is to install all base modules and base dependencies (Odoo core and needed OCA ones) first with no tests (we don't want to test modules that have already been tested.

On that first step, coa module is installed.

Then, launch the tests of this repos modules.

This flow has not to be changed and tests results are successful.

[mt-software-de]

My opinion in that case is, that the tests should be tagged as post_install, because they are (like you have described it) executed as post_install tests.
With executing every test as a post_install test it would be harder to test a auto_install module.
Like sale_stock and procurement_jit, how should this work when there is a similar setup in two oca modules?
Then you have to ensure that a module which is auto_install=True and changes the result of a test, is not inside the odoo addons path.
Otherwise the test could not run through

[rousseldenis]

My opinion in that case is, that the tests should be tagged as post_install, because they are (like you have described it) executed as post_install tests.

So, why all modules in OCA are not tested with 'post-install' tag?

I think you misunderstand the test flow.

• In Odoo:

On the Odoo runbot instance, modules are installed with --test-enable. So, when a module is loaded, tests are run immediately. Except for modules tagged post_install (for the reasons you evoked).

• On OCA:

We install normally all external modules of the repository. Then, we load repository modules with --test-enable.

The difference with Odoo is that all core modules are already installed when we run tests. So, we don't need, in most cases, to tag with post_install our modules.

But, you can for sure use the test flow you want on your side, not on OCA.

[mt-software-de]

Odoo does it like me first install modules with --test-tags -post_install which means only test tagged as at_install
and then in runs the post_install tests

But one thing here again l10n_us and l10n_generic_coa is not a dependency of contract
the dependency is the account modul
and in this module will install l10n in a post_init_hook
so if trying to install the contract module like
odoo-bin -i contract --test-enable -> then the tests will never succeed. Because of the wrongly tagged tests
For me it is fine to close this PR. I will keep it in my case

[rousseldenis]

so if trying to install the contract module like
odoo-bin -i contract --test-enable -> then the tests will never succeed. Because of the wrongly tagged tests

That's because, as I said, you test it without having installed based modules before (and coa module is loaded at the end).

If you want to understand tests flows here, you can read the logs from the 14.0 branch for instance:

https://github.com/OCA/contract/actions/runs/2983787607/jobs/4783850871

You can see there is first an install:

The command used is :

Then, tests are run:

You remark that only the modules of the repository are concerned:

[mt-software-de]

I understand how the oca executes the tests.
One question here:
How you would handle a auto_installed module located in the same repo which changes the results of an other module?

[rousseldenis]

I understand how the oca executes the tests. One question here: How you would handle a auto_installed module located in the same repo which changes the results of an other module?

You want to say two modules that are not aware of each others (no dependency) ?

[mt-software-de]

You want to say two modules that are not aware of each others (no dependency) ?

Yes or one which has only one dependency which is a module of the repo, this could maybe overwrite a method which changes the test results of the parent module

[jbaudoux]

Seems legit

[rousseldenis]

Nope.

[jbaudoux]

@rousseldenis Is it affecting testing on OCA? If not, it does not look wrong to tag it properly like the modules on odoo standard

[rousseldenis]

If not, it does not look wrong to tag it properly like the modules on odoo standard

Why doing this if it is not necessary ? Tests are going well for this module.

[jbaudoux]

If not, it does not look wrong to tag it properly like the modules on odoo standard

Why doing this if it is not necessary ? Tests are going well for this module.

I find it legit to have the module tests succeed when you install it on its own like @mt-software-de explained. I think that what lot of devs are doing when testing locally a module.

[mt-software-de]

How you proceed here, to come to a conclusion?

[rousseldenis]

I find it legit to have the module tests succeed when you install it on its own

post-install tag does not do that!

Look at the test log. post-install will run tests after all modules loaded. That's not the default behaviour and it's unwanted here as not necessary. There is no mean to run a module on its own apart doing a new test job only for that module.

I think that what lot of devs are doing when testing locally a module.

Of course they do, but you can currently test contract module on its own locally without changing anything.

@mt-software-de IMHO, if you have a specific test flow, you should adapt your tests to take that into account, not here.

[mt-software-de]

@rousseldenis i have no specific test flow
the only thing i do is:
creating a new db
installing the base module
then installing the contract module with the option --test-enable or --test-tags=at_install/contract,post_install/contract

This leads to a not successful test run. Because of missing account.journal entries.

In your case or better in the oca test environment it is only working because

At first there is the install the of account and some other modules
which you can see here https://github.com/OCA/contract/actions/runs/2983787607/jobs/4783850871 in step "Initialize test db"
After this the account module is installed
And also the post_init_hook was triggered which sets the modules 10n_us and l10n_generic_coa to "to_install"
And installs the modules at the end of the installation process.

Then there is the installation of contract module with the test-enable flag.
This leads to a successful test run

IMHO: The test should be properly tagged because if you installing this module when there is no module already installed (except base) the test should run successful. And in the case of contract module this could not happen.

[rousseldenis]

IMHO: The test should be properly tagged because if you installing this module when there is no module already installed (except base) the test should run successful. And in the case of contract module this could not happen.

That's the thing I'm explaining from the beginning!

On OCA, the test flow is to install all dependency modules, THEN launching the tests.

So, except for some modules and when it's justified, post-install should not be used.

If you wnat to test it locally, you must respect that flow.

[rousseldenis]

If you wnat to test it locally, you must respect that flow.

The easiest commands to test a module on its own :

odoo -i contract --stop-after-init

odoo -i contract --stop-after-init --test-enable

This works without changing anything.

[mt-software-de]

If you wnat to test it locally, you must respect that flow.

The easiest commands to test a module on its own :

odoo -i contract --stop-after-init

odoo -i contract --stop-after-init --test-enable

This works without changing anything.

Of course this works because
at first you install the module which install the dependencies
and also runs the post_init_hook of account which afterwords creates the account.journals

And then afterwords you run the tests
If you only do this:

odoo -i contract --stop-after-init --test-enable

The tests will fail.
Because this is what happens
Dependencies are getting installed
post_init_hook of account sets the l10n modules "to_install"
contract gets installed
tests of contract are running direct after the install -> data of account.journal is missing
then the auto_install getting installed
as well the l10n modules -> data of account.journals is getting installed

[rousseldenis]

Of course this works because
at first you install the module which install the dependencies
and also runs the post_init_hook of account which afterwords creates the account.journals

And what is refrain you in that flow ?

[rousseldenis]

If you only do this:

odoo -i contract --stop-after-init --test-enable

Why do you do that ?

[mt-software-de]

If you only do this:
odoo -i contract --stop-after-init --test-enable

Why do you do that ?
The short answer is:
Because we want to test the module. :-)
The long one is:
We have a big development team,
everyone in the team is able to change the custom modules, oca modules and odoo core modules.
For example to implement some hooks or change a behavior if they need to.
So thats why we test every installed module with demo data before we deploy new changes

[rousseldenis]

If you only do this:
odoo -i contract --stop-after-init --test-enable

Why do you do that ?
The short answer is:
Because we want to test the module. :-)
The long one is:
We have a big development team,
everyone in the team is able to change the custom modules, oca modules and odoo core modules.
For example to implement some hooks or change a behavior if they need to.
So thats why we test every installed module with demo data before we deploy new changes

As I said, the test flow is like that on OCA, on every repository.

You should test like I exposed before.

[mt-software-de]

I still don't quite understand why it is a wrong to tag the tests properly.
But it is also fine, we will test like we are doing it and there for we will creating a few patches for the unittests.

[rousseldenis]

I still don't quite understand why it is a wrong to tag the tests properly.

Because that tag introduces a different behaviour. It's not 'wrong' but you are changing the way tests are run. This is not needed for that module particularly. And what is not necessary is not needed.

[mt-software-de]

I still don't quite understand why it is a wrong to tag the tests properly.

Because that tag introduces a different behaviour. It's not 'wrong' but you are changing the way tests are run. This is not needed for that module particularly. And what is not necessary is not needed.

Ok. I think it is needed, because if we test it without any pre installed module it will not work. So tagging it properly is necessary.
But either way it is what it is. :-)

[rousseldenis]

if we test it

Who ?

[mt-software-de]

if we test it

Who ?

We with our Development Team

[rousseldenis]

After discussion, this change is acceptable.

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

[rousseldenis]

/ocabot merge patch

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 14.0-ocabot-merge-pr-876-by-rousseldenis-bump-patch, awaiting test results.

[OCA-git-bot]

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