OCA / odoo-community.org

The Odoo Community Association Website
http://odoo-community.org
Creative Commons Attribution 4.0 International
90 stars 123 forks source link

Running tests on post_install #28

Open rousseldenis opened 6 years ago

rousseldenis commented 6 years ago

As debated here : https://github.com/OCA/server-ux/pull/32 and https://github.com/OCA/delivery-carrier/pull/167#discussion_r226293533 I want to have clear guidelines on how to run tests on OCA repositories. At post install, at install, or ... because it can leads to unproductive threads between different modules maintainers.

Many thanks for your feedback, experience, ...

rousseldenis commented 6 years ago

cc @OCA/core-maintainers

rousseldenis commented 6 years ago

A little bit reading https://en.wikipedia.org/wiki/Unit_testing

pedrobaeza commented 6 years ago

These are tests, not unit tests. You give that meaning with your approach.

https://www.guru99.com/unit-test-vs-integration-test.html

rousseldenis commented 6 years ago

We can't do integration tests in OCA repositories at module level ! We have been always done unit tests. Or am I wrong ?

pedrobaeza commented 6 years ago

We are not talking about doing integration tests at OCA level, but on integrator level, but OCA modules mustn't forbid this possibility. And the same tests are used for integration tests.

dreispt commented 6 years ago

I don't have the full discussion context, but as general rule OCA repos should test all the modules installed together. The point is to ensure that they work correctly together (or at least they don't break each other). This compatibility is an important feature for OCA Stable modules.

elicoidal commented 6 years ago

I am wondering whether this repository is the right one for the discussion: do you think we should have a separate document with the guidelines or would you like to have the discussion first here and then push a guideline? If not this repo, actually I am not sure which one would be more relevant I reckon cc @rousseldenis @OCA/board

sbidoul commented 6 years ago

@elicoidal I suggested to raise the discussion here because a guideline is needed and the guideline will end up in CONTRIBUTING.rst in this repo.

yvaucher commented 6 years ago

I also think, we are more closer in OCA to unit test than integration tests.

However, the function we are testing have a lot of inputs (a lot of records to prepare) thus it is simpler to do the workflow instead of mocking every inputs.

That's why I advocate to skip the code in test context with check on test-enable to not force other modules' tests to run it. But maybe this could be an opt-in option in res.config.settings making the default workflow the first to be in use in other modules tests. Which would force you to install a module + enable an option.

@dreispt I'm not sure if you advocate for more resilient tests or to just ensure that modules don't break others with fairness. Resilient tests would force us to change a lot of tests if one module goes off track from a standard workflow unless it is isolated. I'm not in favor of this. Unless you are talking about having modules being fair to other modules and always offer a way to not break other tests especially on enforcing a different workflow. By default when running a test you should be on the standard workflow and don't have to care that if module A or B is installed. We might want to enforce more opt-in settings in modules.

Of course fairness in module design doesn't help on testing module compatibility. It will just show that no module is fighting against the others' tests.

For more testing especially integration tests to check compatibility of modules with all activated features, I would suggest to create glue modules containing tests that enable all features and proves two or more independent modules are working well together. In some case it could be very useful. And there you would explicitly be testing the unrelated modules.

IMO when creating a module, a rule of thumb should be that standard workflow can always be followed by other modules' tests by default. And if your module doesn't comply it deserves test isolation.

dreispt commented 6 years ago

@dreispt I'm not sure if you advocate for more resilient tests or to just ensure that modules don't break others with fairness.

Ensuring that modules don't break other, as a general rule, is a basic requirement, and the main reason why we should have functional-area focused repos.

Integration tests involving several different modules would be an ideal scenario, but I agree with you that it's not easy and add that there's not much motivation for that when the modules involved have different authors. Yes, "glue" test modules for integration tests would be a way for this, so no technical impediment for actually doing it.

IMO when creating a module, a rule of thumb should be that standard workflow can always be followed by other modules' tests by default. And if your module doesn't comply it deserves test isolation.

Absolutely. That perfectly sums up my point.

lmignon commented 6 years ago

I would like to add an additional argument to respect the normal flow in the execution of tests. It happens regularly in my developments to run the tests of a single module after having modified it. In some cases, I discover that even without my modifications, the tests fail because the code tries to access fields or methods that are not present in dependencies. These errors are systematically masked by our CI since the addons providing these stuff are installed by others addons. If the tests are executed in post_install, this kind of bugs will also be masked.... The only way to detect this kind of error is to ensure proper test isolation.

pedrobaeza commented 6 years ago

But this is something that you are not going to fully avoid removing post_install: it will depend on the module loading order.

lmignon commented 6 years ago

In the best world, each module should be tested in its own env with only these dependencies installed. Even if with our large repositories it's not possible to fully avoid this but with post_install we are sure that this kind of error will no more be detected.

sbidoul commented 6 years ago

Would it be a sensible guideline to say that OCA module that impose restrictions or changes on standard flows must have a flag (off by default) to enable that behaviour?

In any case, I don't see a good reason to have all OCA tests having the post_install flag by default. With my superficial view on the issue, it seems to me that it only postpones the problem.

dreispt commented 6 years ago

FYI in my MQT earlier work I included a "unit test" build where each module was individually tested. This was considered not necessary buy other MQT maintainers, and has been kind of deprecated.

pedrobaeza commented 6 years ago

The problem was not the need of it, but the time it consumes.

rousseldenis commented 5 years ago

As I triggered this, I want to know if we can summarize the result of this thread.

As repos are grouping most modules allowing to extend functionnalities in a same way, but as OCA development involves many authors that expect their modules working not necessarily with all of those contained in the repo, letting the default behaviour (at_install) should be the more 'convenient' way not to exhaust contributors to fix neverending errors.

And to ensure adequation between two fighting ones should be developping glue modules, options to enable changing behaviour, or ...

That way could lead to more independance (as for now) between modules.

Is this could be inserted into development guideline ?

yajo commented 3 years ago

I just opened #63 and then later I found this thread, so you might be interested in reviewing that PR, which clarifies the situation in the specific case of website modules for Odoo v12+.