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

[16.0][MIG] edi_party_data_oca: Migration to 16.0 #20

Merged
merged 10 commits into from
Oct 18, 2023

Conversation

QuocDuong1306
Copy link

@QuocDuong1306 QuocDuong1306 commented Sep 23, 2023

Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG overall

@@ -0,0 +1,2 @@
odoo_test_helper
odoo-addon-edi_oca @ git+https://github.com/OCA/edi-framework.git@refs/pull/5/head#subdirectory=setup/edi_oca
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can drop this req

@QuocDuong1306 QuocDuong1306 marked this pull request as draft October 4, 2023 04:48
@QuocDuong1306 QuocDuong1306 mentioned this pull request Oct 9, 2023
3 tasks
@nilshamerlinck
Copy link
Contributor

A few notes about edi_party_data_oca tests:

  1. if we only run them alone, they pass
  2. if we run tests for both eci_oca and edi_party_data_oca, they fail
  3. in 14.0, they pass, but that's because @tagged("-at_install", "post_install") is not actually effective: tests run at_install (TODO)
  4. in 16.0, @tagged("-at_install", "post_install") works so tests run on a fully loaded registry
  • then FakeModelLoader is used in 2 edi_oca tests: test_consumer_mixin, test_security
  • and it impacts the registry on registry.load("edi_oca") in both update_registry() and restore_registry(): env["edi.exchange.type"] registry class is rebuilt ignoring edi_party_data_oca's extension
  • as a consequence, we get the error: AttributeError: 'edi.exchange.type' object has no attribute 'id_category_ids'
  1. workarounds:
  • run edi_party_data_oca tests before edi_oca tests using @tagged("at_install", "-post_install")
  • force reload of edi_party_data_oca with FakeModelLoader (current chosen workaround)
  1. long term fixes:
  • it seems to me that the second registry.load("edi_oca") in restore_registry() is not needed anymore in odoo >= 16.0, to be investigated further (inputs welcome!)

@QuocDuong1306 QuocDuong1306 marked this pull request as ready for review October 9, 2023 04:31
@simahawk
Copy link
Contributor

simahawk commented Oct 9, 2023

@nilshamerlinck no time to look into details now... Can this help? OCA/odoo-test-helper#27

@nilshamerlinck
Copy link
Contributor

Hello @simahawk no, it's a different case; anyway, PR is green now with the workaround :) long term fixes can/will come later

@simahawk
Copy link
Contributor

simahawk commented Oct 9, 2023

/ocabot migration edi_party_data_oca

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Oct 9, 2023
@OCA-git-bot
Copy link
Contributor

There's no issue in this repo with the title 'Migration to version 16.0' and the milestone 16.0, so not possible to add the comment.

@nilshamerlinck
Copy link
Contributor

Hello @simahawk, opened OCA/odoo-test-helper#28 to address the tests issues at the odoo-test-helper level instead of workarounds in impacted PRs

@etobella
Copy link
Member

TEST Requirements are not needed, the branch has already been merged and deployed 😄

@simahawk
Copy link
Contributor

odoo-test-helper 2.1.1 released

test-requirements.txt Outdated Show resolved Hide resolved
@QuocDuong1306
Copy link
Author

It's OK, thank @simahawk @etobella

@simahawk
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-20-by-simahawk-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 1850cc0 into OCA:16.0 Oct 18, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at f8cfa53. Thanks a lot for contributing to OCA. ❤️

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

Successfully merging this pull request may close these issues.

5 participants