Skip to Content
  2. Mailing Lists
  3. Contributors
  4. Re: Tests failure when invoicing is involved after a change in Odoo upstream

Archives

Contributors

Re: Tests failure when invoicing is involved after a change in Odoo upstream

Re: Tests failure when invoicing is involved after a change in Odoo upstream

Re: Tests failure when invoicing is involved after a change in Odoo upstream

Stefan and Denis, the reason for the Odoo patch may be legit, and it is. The problem is that it comes too late for the lifecycle of this version IMO, and more having in version 17 integrated the generic CoA into account for better practices. Anyway, this is just undisclosing a problem that has been hidden in the design of our OCA tests, them not being deterministic and really unit tests, as they are relying on previous state of data for being successful:

- The company used is the one populated by default instead of using a full set of data including the company. This may be reasonable for execution times, and that's why the decision to just fix this condition installing an existing CoA instead of populating on each test a new company.
- There's no guarantee a CoA is loaded at the time of performing the tests, but creating an invoice requires it. That's why the post-install thing (and it's also a requirement if inheriting AccountTestInvoicingCommon).


Inheriting from AccountTestInvoicingCommon, as explained in the commit message I linked in my first message, it's not improving the thing (at least in 16, in 17 it's more ironized), because it still requires `l10n_generic_coa` installed (and installed before the execution of this test), and its setup is very heavy, creating 2 companies (even if you only require one), so this would increase the test execution times a lot in the best case, or skipped without you really noticing in the worst (when generic CoA is not available).

The added code is NOT boilerplate. It's legit code for making deterministic tests, the same as you may remove currency rates for having a deterministic environment. And it's totally harmless if you don't need it, because starting with, it's in test code, not in business code, and if you keep the previous status quo, the if it's not executed, so it doesn't affect any previous condition. This can only uncover other hidden defects on the modules being executed in post-install, and this is being patched at the same time.

Replying to other threads answers, I don't think this will reverted, as AFAIK Odoo.sh is auto-installing the modules that hides the problem, and not installing any localization module, but I'm not expert of that platform, so I may be wrong.

Regards.
by Pedro M. Baeza - 04:52 - 26 Feb 2025

Reference

  • Tests failure when invoicing is involved after a change in Odoo upstream
    I write to inform you that Odoo has performed a change in "stable" 16.0 in this commit:


    that is affecting CIs, specially on those integration ones that OCA implementers perform combining all the modules of a specific instance.

    This change avoids the auto-install of one CoA on the existing demo main company, as it doesn't comply with a country CoA unless you have l10n_us installed. Some tests inheriting from  AccountTestInvoicingCommon won't fail if l10n_generic_coa is installed, but it's also a requirement that can't always be fulfilled.

    The impact of this is huge across OCA, as basically every test creating an invoice is affected. OCA CI may not be red, but use it in other place and you may be doomed.


    Thus, from Tecnativa, we have started a massive pull request campaign fixing most of the common modules with this problem.

    The strategy has been to force a load of the generic CoA or the first existing one into the main company if starting the test, it doesn't have any. Check the minimum example at:


    If you are inheriting from AccountTestInvoicingCommon, the strategy should be different. By default, you won't have a red CI, but the tests are skipped, which is also undesirable. Thus, you have to do the work before calling super. Check an example here:


    v17 is not affected for now. Let's hope it continues this way...

    Regards.
    by Pedro M. Baeza - 03:41 - 25 Feb 2025

    7 replies 7 replies

    • Re: Tests failure when invoicing is involved after a change in Odoo upstream
      Well, that only fixes the problem in specific repositories OCA CIs, and you still needed to make some changes in tests. The problem we had was when using the modules in our own Ci, and the proposed patch was a good balance for not disturbing current OCA status and don't fail in our scenario.

      Regards.
      by Pedro M. Baeza - 05:07 - 12 Mar 2025
    • Re: Tests failure when invoicing is involved after a change in Odoo upstream
      While fixing the 16.0 branch for l10n_belgium I have done some more research on this.

      In 16.0, something like `odoo -i l10n_be` gives you two companies with main company without CoA (that's the problem mentioned in OP)

      - Your Company (base.main_company), country=US, no CoA
      - BE Company, country=BE, BE CoA

      In 17.0 and 18.0, the generic CoA is loaded on main_company even though a localization was loaded:

      - Your Company (base.main_company), country=US, Generic CoA
      - BE Company, country=BE, BE CoA

      This is why we only see the problem in 16.

      So I now think pre-loading l10n_generic_coa before running the tests in 16.0 is an easy fix, and also a good solution (and the one I prefer), because it sets up the main company identically to what we get in later versions.

      IMO, tests that really depend on a CoA should use a company initialized with that CoA, and not base.main_company.

      This is what I did in l10n_belgium: https://github.com/OCA/l10n-belgium/pull/243

      Best regards,

      -Stéphane

      On Fri, Feb 28, 2025 at 7:33 PM Stéphane Bidoul <stephane.bidoul@acsone.eu> wrote:
      I have hit this issue in one of my projects where I run all tests post-install.

      Instead of changing all my tests, I chose to install l10n_generic_coa in CI, which was an easy solution in my specific situation.
      Maybe this can help others hitting the same problem.

      Best regards,

      -Stéphane


      On Wed, Feb 26, 2025 at 4:53 PM Pedro M. Baeza <notifications@odoo-community.org> wrote:
      Stefan and Denis, the reason for the Odoo patch may be legit, and it is. The problem is that it comes too late for the lifecycle of this version IMO, and more having in version 17 integrated the generic CoA into account for better practices. Anyway, this is just undisclosing a problem that has been hidden in the design of our OCA tests, them not being deterministic and really unit tests, as they are relying on previous state of data for being successful:

      - The company used is the one populated by default instead of using a full set of data including the company. This may be reasonable for execution times, and that's why the decision to just fix this condition installing an existing CoA instead of populating on each test a new company.
      - There's no guarantee a CoA is loaded at the time of performing the tests, but creating an invoice requires it. That's why the post-install thing (and it's also a requirement if inheriting AccountTestInvoicingCommon).


      Inheriting from AccountTestInvoicingCommon, as explained in the commit message I linked in my first message, it's not improving the thing (at least in 16, in 17 it's more ironized), because it still requires `l10n_generic_coa` installed (and installed before the execution of this test), and its setup is very heavy, creating 2 companies (even if you only require one), so this would increase the test execution times a lot in the best case, or skipped without you really noticing in the worst (when generic CoA is not available).

      The added code is NOT boilerplate. It's legit code for making deterministic tests, the same as you may remove currency rates for having a deterministic environment. And it's totally harmless if you don't need it, because starting with, it's in test code, not in business code, and if you keep the previous status quo, the if it's not executed, so it doesn't affect any previous condition. This can only uncover other hidden defects on the modules being executed in post-install, and this is being patched at the same time.

      Replying to other threads answers, I don't think this will reverted, as AFAIK Odoo.sh is auto-installing the modules that hides the problem, and not installing any localization module, but I'm not expert of that platform, so I may be wrong.

      Regards.

      _______________________________________________
      Mailing-List: https://odoo-community.org/groups/contributors-15
      Post to: mailto:contributors@odoo-community.org
      Unsubscribe: https://odoo-community.org/groups?unsubscribe

      by Stéphane Bidoul - 04:11 - 12 Mar 2025
    • Re: Tests failure when invoicing is involved after a change in Odoo upstream
      I have hit this issue in one of my projects where I run all tests post-install.

      Instead of changing all my tests, I chose to install l10n_generic_coa in CI, which was an easy solution in my specific situation.
      Maybe this can help others hitting the same problem.

      Best regards,

      -Stéphane


      On Wed, Feb 26, 2025 at 4:53 PM Pedro M. Baeza <notifications@odoo-community.org> wrote:
      Stefan and Denis, the reason for the Odoo patch may be legit, and it is. The problem is that it comes too late for the lifecycle of this version IMO, and more having in version 17 integrated the generic CoA into account for better practices. Anyway, this is just undisclosing a problem that has been hidden in the design of our OCA tests, them not being deterministic and really unit tests, as they are relying on previous state of data for being successful:

      - The company used is the one populated by default instead of using a full set of data including the company. This may be reasonable for execution times, and that's why the decision to just fix this condition installing an existing CoA instead of populating on each test a new company.
      - There's no guarantee a CoA is loaded at the time of performing the tests, but creating an invoice requires it. That's why the post-install thing (and it's also a requirement if inheriting AccountTestInvoicingCommon).


      Inheriting from AccountTestInvoicingCommon, as explained in the commit message I linked in my first message, it's not improving the thing (at least in 16, in 17 it's more ironized), because it still requires `l10n_generic_coa` installed (and installed before the execution of this test), and its setup is very heavy, creating 2 companies (even if you only require one), so this would increase the test execution times a lot in the best case, or skipped without you really noticing in the worst (when generic CoA is not available).

      The added code is NOT boilerplate. It's legit code for making deterministic tests, the same as you may remove currency rates for having a deterministic environment. And it's totally harmless if you don't need it, because starting with, it's in test code, not in business code, and if you keep the previous status quo, the if it's not executed, so it doesn't affect any previous condition. This can only uncover other hidden defects on the modules being executed in post-install, and this is being patched at the same time.

      Replying to other threads answers, I don't think this will reverted, as AFAIK Odoo.sh is auto-installing the modules that hides the problem, and not installing any localization module, but I'm not expert of that platform, so I may be wrong.

      Regards.

      _______________________________________________
      Mailing-List: https://odoo-community.org/groups/contributors-15
      Post to: mailto:contributors@odoo-community.org
      Unsubscribe: https://odoo-community.org/groups?unsubscribe

      by Stéphane Bidoul - 07:36 - 28 Feb 2025
    • Re: Tests failure when invoicing is involved after a change in Odoo upstream
      Stefan and Denis, the reason for the Odoo patch may be legit, and it is. The problem is that it comes too late for the lifecycle of this version IMO, and more having in version 17 integrated the generic CoA into account for better practices. Anyway, this is just undisclosing a problem that has been hidden in the design of our OCA tests, them not being deterministic and really unit tests, as they are relying on previous state of data for being successful:

      - The company used is the one populated by default instead of using a full set of data including the company. This may be reasonable for execution times, and that's why the decision to just fix this condition installing an existing CoA instead of populating on each test a new company.
      - There's no guarantee a CoA is loaded at the time of performing the tests, but creating an invoice requires it. That's why the post-install thing (and it's also a requirement if inheriting AccountTestInvoicingCommon).


      Inheriting from AccountTestInvoicingCommon, as explained in the commit message I linked in my first message, it's not improving the thing (at least in 16, in 17 it's more ironized), because it still requires `l10n_generic_coa` installed (and installed before the execution of this test), and its setup is very heavy, creating 2 companies (even if you only require one), so this would increase the test execution times a lot in the best case, or skipped without you really noticing in the worst (when generic CoA is not available).

      The added code is NOT boilerplate. It's legit code for making deterministic tests, the same as you may remove currency rates for having a deterministic environment. And it's totally harmless if you don't need it, because starting with, it's in test code, not in business code, and if you keep the previous status quo, the if it's not executed, so it doesn't affect any previous condition. This can only uncover other hidden defects on the modules being executed in post-install, and this is being patched at the same time.

      Replying to other threads answers, I don't think this will reverted, as AFAIK Odoo.sh is auto-installing the modules that hides the problem, and not installing any localization module, but I'm not expert of that platform, so I may be wrong.

      Regards.
      by Pedro M. Baeza - 04:52 - 26 Feb 2025
    • Re: Tests failure when invoicing is involved after a change in Odoo upstream
      Thanks Pedro for pointing that out.

      But as Stefan said, the changes you propose should be applied in particular modules that do pull particular localisation, not every.

      As said also, a lead could be using AccountTestInvoicingCommon.

      Moreover, I think proposing a hook in Odoo's tests should be great to lower the code we need to introduce in our modules, or... What do you think?

      Thanks

      Le mer. 26 févr. 2025, 11:27, Stefan Rijnhart <notifications@odoo-community.org> a écrit :
      Yes, but the reason given by the Odoo devs for this change (https://github.com/odoo/odoo/pull/194799) is that CI can break on other CoAs than the generic one. That implies that it is considered bad practise to encourage it, and maybe we should not be doing this in (potentially, a lot of) OCA modules. Maybe using AccountTestInvoicingCommon is still the best solution?

      Like you say, OCA CI should mostly not be affected because l10n_generic_coa is still installed automatically if you don't depend on a localized chart. Do you really *need* to test these modules on databases with l10n_es? Maybe you can enforce the installation of it on the main company with an extra step in your CI.

      On 25-02-2025 15:42, Pedro M. Baeza wrote:
      I write to inform you that Odoo has performed a change in "stable" 16.0 in this commit:


      that is affecting CIs, specially on those integration ones that OCA implementers perform combining all the modules of a specific instance.

      This change avoids the auto-install of one CoA on the existing demo main company, as it doesn't comply with a country CoA unless you have l10n_us installed. Some tests inheriting from  AccountTestInvoicingCommon won't fail if l10n_generic_coa is installed, but it's also a requirement that can't always be fulfilled.

      The impact of this is huge across OCA, as basically every test creating an invoice is affected. OCA CI may not be red, but use it in other place and you may be doomed.


      Thus, from Tecnativa, we have started a massive pull request campaign fixing most of the common modules with this problem.

      The strategy has been to force a load of the generic CoA or the first existing one into the main company if starting the test, it doesn't have any. Check the minimum example at:


      If you are inheriting from AccountTestInvoicingCommon, the strategy should be different. By default, you won't have a red CI, but the tests are skipped, which is also undesirable. Thus, you have to do the work before calling super. Check an example here:


      v17 is not affected for now. Let's hope it continues this way...

      Regards.

      _______________________________________________
      Mailing-List: https://odoo-community.org/groups/contributors-15
      Post to: mailto:contributors@odoo-community.org
      Unsubscribe: https://odoo-community.org/groups?unsubscribe



      -- 
Opener B.V. - Business solutions driven by open source collaboration

Stefan Rijnhart - Consultant/developer

mail: stefan@opener.amsterdam
tel: +31 (0) 6 1447 8606
web: https://opener.amsterdam

      _______________________________________________
      Mailing-List: https://odoo-community.org/groups/contributors-15
      Post to: mailto:contributors@odoo-community.org
      Unsubscribe: https://odoo-community.org/groups?unsubscribe

      by Denis Roussel - 04:21 - 26 Feb 2025