odoo / odoo

Odoo. Open Source Apps To Grow Your Business.
https://www.odoo.com
Other
38.74k stars 25.09k forks source link

[FR] add test_dependencies to modules manifest #40188

Closed yajo closed 2 years ago

yajo commented 4 years ago

Impacted versions: all

Current behavior: Sometimes a module needs another one to be installed to be able to execute its tests, while it's not a strict dependency for its normal usage. A workaround to execute those tests is to move those tests to post-install mode, but that has its own problems because an uncontrolled module in the database can break tests behavior.

Requested behavior: In __manifest__.py we should be able to declare test_dependencies with a list of modules that have a dependency for running tests. It would do nothing when on a normal install/update process, but when --test-enable is passed, then those would be concatenated with normal dependencies and processed as such.

One simple example are modules that depend on accounting. Those would be much easier to test if added "test_dependencies": ["l10n_generic_coa"], when on a normal DB that module wouldn't be necessary.

Downstream modules that extend mail could add "test_dependencies": ["test_mail"] to make sure the testing models are in place before running tests.

Yenthe666 commented 4 years ago

So in short: if you're using --test-enable you'd propose to install the dependencies for those tests while ignoring the dependencies if not run through --test-enable?

yajo commented 4 years ago

Yes, basically.

Dependencies affect module installation order and module availability in the test scope (when at-install mode), so that would let us write smarter tests with less difficulties.

Yenthe666 commented 4 years ago

@xmo-odoo and @rco-odoo perhaps you guys can shed some light/ideas on this new idea for the testing framework? :)

yelizariev commented 4 years ago

I need such feature too

yajo commented 4 years ago

Let me ping @Xavier-Do, as he was the one who explained why https://github.com/odoo/odoo/pull/24833 wasn't a good solution for this problem. I think this one is better.

simahawk commented 4 years ago

IMO at the same time we should have external_test_dependencies to declare python test-only deps.

simahawk commented 3 years ago

@Xavier-Do @xmo-odoo @rco-odoo any feedback on this?

bealdav commented 3 years ago

It could really improve odoo modules landscape

etobella commented 3 years ago

Great Idea!!! That would be awesome :smile:

em230418 commented 3 years ago

As a workaround, make other module just for testing only.

Xavier-Do commented 3 years ago

What you actually want is a more a test_auto_install than a test_dependencies. Your test_mail example would create a cyclic dependency otherwise since test_mail depends on mail.

Anyway, I'm not sure it is a good idea, we actually want to keep install and test logic as independant as possible. Since 14.0 it is possible to use test_enable without any install or update. This means that you can install a module and then test it in a separate command (especially true for post_install but also works for at_install most of the time). This is used on runbot to parallelize tests, but also to debug/write tests and to be able to execute them without waiting for an update/install.

The problem with this solution is that -d test -i mail followed by -d test --test-tags /mail wouldn't have the same effect anymore than -d test -i mail --test-tags /mail, since a new module would be installed. And in some case you don't want to install test_mail even if test are active: -i crm --test-tags /crm. I don't need test_mail in this case, but since mail is installed and test are enabled, it would be the case, slowing down local testing process.

I don't know why l10n_generic_coa would be needed for account testing to be honnest, but this may lead to another problem. When tests are enable, this module would be added, meaning that if for some reason some account logic depends on l10n_generic_coa, it will be almost impossible to notice. (single modules builds running the weekend are especially done for that) Account in this case should be tested by himsel. If some logic needs both account and l10n_generic_coa to be tested, a test module can be used.

yajo commented 2 years ago

Spoke today with @VincentSchippefilt about this. He convinced me this cannot be a good solution.

The dependencies are very important for how a module works, as they impact on how fields and methods overrides are ordered. By doing what I proposed here, we'd have a different dependency graph for production and testing. That could lead to different behaviors of the same code depending on the environment. Bad idea.

So the only solution is to add a separate test module that includes the test dependencies and the tests. And don't install it in production. Just like odoo does.

Thanks!