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

[TEST] The module help_online has incorrect dependencies. #197

Closed
nhomar opened this issue Aug 6, 2015 · 13 comments
Closed

[TEST] The module help_online has incorrect dependencies. #197

nhomar opened this issue Aug 6, 2015 · 13 comments
Labels

Comments

@nhomar
Copy link
Member

nhomar commented Aug 6, 2015

The module help_online depende of anybox for testing, is that ok for us? when you run with test enabled and such modules intitated you receive this message:

2015-08-06 07:00:37,218 26341 ERROR vauxoo openerp.modules.module: Can not `import help_online`.
Traceback (most recent call last):
  File "/Users/nhomar/Trabajo/odoo80/openerp/modules/module.py", line 377, in get_test_modules
    mod = importlib.import_module('.tests', modpath)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/Users/nhomar/Trabajo/web/help_online/tests/__init__.py", line 20, in <module>
    from . import test_export_help_wizard
  File "/Users/nhomar/Trabajo/web/help_online/tests/test_export_help_wizard.py", line 25, in <module>
    from anybox.testing.openerp import SharedSetupTransactionCase
ImportError: No module named anybox.testing.openerp
@nhomar nhomar added the question label Aug 6, 2015
@pedrobaeza
Copy link
Member

We shouldn't depend in anything outside OCA, so at first instance, it should be not.

@nhomar
Copy link
Member Author

nhomar commented Aug 6, 2015

Ok, then I will try to remove such tests and include some of them using nose (de official one)

@lmignon
Copy link
Contributor

lmignon commented Aug 6, 2015

@pedrobaeza, the depency rules is for modules not for python lib. anybox.testing.openerp is a pythonlib that ease the way to write tests for Odoo. IMO, the rule is not applicable to this case.

@lmignon
Copy link
Contributor

lmignon commented Aug 6, 2015

@nhomar IMO, you don't need to refactor the tests. You just need to install anybox.testing.openerp with pip

@pedrobaeza
Copy link
Member

OK, I didn't know about pip package existence. Sorry for the confusion.

@nhomar
Copy link
Member Author

nhomar commented Aug 6, 2015

I know with a pip it can be solved, no issue with that, but until the rule is saves and anybox tests are under the OCA maintainance there is no way to have modules with such external dependencies which are redundant function from the core ones.

nosetest can do the same job (in a different way) but it can.

@nhomar
Copy link
Member Author

nhomar commented Aug 6, 2015

I read your test, and all what you do there with anybox can be done for sure with base test case. Can you enlight me what is the real difference between the tool you used and the unittest2 or nose?

@lmignon
Copy link
Contributor

lmignon commented Aug 6, 2015

@nhomar SharedSetupTransactionCase inherit from openerp.tests.common.SingleTransactionCase that inherit from BaseCase that inherit fom unittest2.TestCase. Therefore is a conventional unittest2.
SharedSetupTransactionCase provides a way to reduce the time required to run tests. It's not redundant to the core it's an enhancement of what is provided by the core. see https://bitbucket.org/anybox/anybox.testing.openerp#rst-header-sharedsetuptransactioncase

but until the rule is saves and anybox tests are under the OCA maintainance

Which rule? anybox.testing.openerp is a pythonlib. Do you expect that all the pythonlibs used by Odoo should be under the control of OCA or Odoo? (requests, libxml, ....)

IMO it's great to be able to use some lib that ease the way we can write tests.

@nhomar
Copy link
Member Author

nhomar commented Aug 6, 2015

2015-08-06 3:35 GMT-05:00 Laurent Mignon (ACSONE) notifications@github.com
:

Which rule? anybox.testing.openerp is a pythonlib. Do you expect that all
the pythonlibs used by Odoo should be under the control of OCA or Odoo?
(requests, libxml, ....)

No.

What I think is simple and straight forward.

IF: The extralib give me an really good enhancement.... coo! I installit
and all ok.

But If we have declared one repository and only for 1 tests which can be
done with base I need to dedicate to make a pip install with something not
which I do not know why it adds value, I stop and think.

And in this case I could not find any parst of your test that REALLY need
such library, all of it can be done without such dependency on the test.

I do not know if I explain myself better?

It is not a matter of:

Use or not Use because it is easy or not to install.

It is a matter of:

Let's use if we really need, because every package you add is a lot of
external thing over every one of us do not have contrl over it... then...
think it twice.

Regards.


Saludos Cordiales

CEO at Vauxoo https://www.vauxoo.com Odoo's Gold Partner.

[image: --]
Nhomar Hernandez
[image: http://]about.me/nhomar
http://about.me/nhomar?promo=email_sig

@lmignon
Copy link
Contributor

lmignon commented Aug 7, 2015

@nhomar you are free to change the code. The module has been developed in the context of a large project where we taken advantage of the functionalities provided by the SharedSetupTransactionCase to speed up our tests and ease their developments. The choice of using this library in our projects is a maturely considered choice.

@sbidoul
Copy link
Member

sbidoul commented Aug 15, 2015

Can't this be solved by simply adding anybox.testing.openerp in external_dependencies ?

@nhomar
Copy link
Member Author

nhomar commented Aug 15, 2015

@sbidoul

Yes, but the point is not which external dependency we should add, the point is that If the dependency really add a value in an specific OCA module then cool.

On this especific case and just on this specific case (I am not generalizing) it do not add any, because even such tests are possible to be done without such dependency.

And the impact of add an extra dependency is not always as simple as a sudo pip install everytime you decide use an external dependency you need to think that the one that will be using it sooner or later will need to mantain or add it to its deployment infraestructure, then such job (which is really necesary in some cases) need to be correctly supported.

In OCA, we encourage make tests with normal base ones, if the rule will be use anybox.testing.openerpfor any reason, then cool, we follow such lead, but in this case I actually do not understand why for one set of 2 or three modules which are using that I will change the deploy strategy of everybody and teh testing techniques.

I did not find "yet" teh technical reason to include such effort.

But BTW the module is there, as I mentioned, I will do it in some moment for now... No there are any issue with that.

I hope it clarify a little my point.

Regards.

@pedrobaeza
Copy link
Member

As there's no PR changing tests, I close the issue.

vrenaville pushed a commit to camptocamp/web that referenced this issue Jul 19, 2018
BSMTS-308 Add missing i18n for analysis report footer + center text
davidtranhp pushed a commit to davidtranhp/web that referenced this issue Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants