Skip to content
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

Running tests on post_install #28

Open
rousseldenis opened this issue Oct 18, 2018 · 18 comments
Open

Running tests on post_install #28

rousseldenis opened this issue Oct 18, 2018 · 18 comments

Comments

@rousseldenis
Copy link

As debated here : OCA/server-ux#32 and OCA/delivery-carrier#167 (comment) 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
Copy link
Author

cc @OCA/core-maintainers

@rousseldenis
Copy link
Author

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

@pedrobaeza
Copy link
Member

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

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

@rousseldenis
Copy link
Author

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

@pedrobaeza
Copy link
Member

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
Copy link
Member

dreispt commented Oct 18, 2018

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
Copy link
Contributor

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
Copy link
Member

sbidoul commented Oct 18, 2018

@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
Copy link
Member

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
Copy link
Member

dreispt commented Oct 23, 2018

@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
Copy link

lmignon commented Nov 2, 2018

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
Copy link
Member

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

@lmignon
Copy link

lmignon commented Nov 2, 2018

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
Copy link
Member

sbidoul commented Nov 2, 2018

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
Copy link
Member

dreispt commented Nov 2, 2018

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
Copy link
Member

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

@rousseldenis
Copy link
Author

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
Copy link
Member

yajo commented Jul 13, 2021

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+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants