[16.0] Adapter test refactor + fix each method #211
Conversation
…ring For now only move elastic test into a common class The idea is to have the same test for all adapter as all adapter should have the same behaviour
Now adapter have a generic testing flow all adapter should behave exactly in a same way additionnal test can be included to test specific case like error catching
sebastienbeau
force-pushed
the
16.0-adapter-test-refactor
branch
from
89a3dac to
6ebf356
Compare
sebastienbeau
changed the title
sebastienbeau
force-pushed
the
16.0-adapter-test-refactor
branch
from
55468ee to
044b4ce
Compare
|
@paradoxxxzero can you check ? |
sebastienbeau
changed the title
sebastienbeau
force-pushed
the
16.0-adapter-test-refactor
branch
from
07e571c to
044b4ce
Compare
lmignon
left a comment
lmignon
left a comment
There was a problem hiding this comment.
Good idea. A little comment...
| binding = binding_model.browse(ext_id).exists() | ||
| if not binding: | ||
| item_ids.append(ext_id) | ||
| for index_record in adapter.each(fetch_fields=["id"]): |
There was a problem hiding this comment.
@sebastienbeau If you modify the method signature, youmust modify the related ABC class https://github.com/OCA/search-engine/blob/16.0/connector_search_engine/tools/adapter.py#L41
There was a problem hiding this comment.
Thanks for the catch !!!
I fix it :
https://github.com/OCA/search-engine/pull/211/files#diff-0999632348627c0ffd09fd3aa1bbe05571cca744693515a5a7d28e92d12cb722R41
| adapter = self.se_adapter | ||
| binding_model = self.env[index.model_id.model] | ||
| for index_record in adapter.each(): | ||
| ext_id = adapter._get_odoo_id_from_index_data(index_record) |
There was a problem hiding this comment.
IMO it's important to keep this method. We don't want to hardcode the field used to store the id into the index.
There was a problem hiding this comment.
My idea, but maybe it's not a good one, was to fix a wrong design that have introduced initially with the "_record_id_key" and that have been refactored
Some context:
- Elastic or Typesense: the search engine use "id" as ID
- Algolia it use "ObjectID"
Right now for Algolia (If we migrate with the current API)
adapter.index({'id': 1, "name": "Foo", "ObjectID": 1})
adapter.delete([1])
adapter.each() => [{"id": 1, "name": "Foo", "ObjectID": 1}]
So we need "outside" of the adapter
- to inject the key ObjectID before doing the index
- to call the method _get_odoo_id_from_index_data after reading data
With the change
I think it's better to move the logic of switching the right id inside the adapter and use the adapter like that
adapter.index({'id': 1, "name": "Foo"})
adapter.delete([1])
adapter.each() => [{"id": 1, "name": "Foo"}]
So the idea is to add a specific logic in the adapter for search engine that do not use "id" as keyword. So mostly it will need some adaptation in the Algolia connector
What do you think ?
sebastienbeau
force-pushed
the
16.0-adapter-test-refactor
branch
from
044b4ce to
d01c447
Compare
fix broken "def each" method for elastic and implement addional test make it work even with corrupted elastic index
sebastienbeau
force-pushed
the
16.0-adapter-test-refactor
branch
from
d01c447 to
e6cc527
Compare
Co-authored-by: Laurent Mignon (ACSONE) <[email protected]>
sebastienbeau
force-pushed
the
16.0-adapter-test-refactor
branch
from
36017d9 to
bc1498e
Compare
|
@lmignon Now it's ready ! can you update your review ? |
|
/ocabot merge major |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at 9962bdc. Thanks a lot for contributing to OCA. ❤️ |
3 participants
Right now each search engine implement its own test for testing the adapter.
As we start to have other contribution of search engine. A wrong test can make an adapter having a different behavior.
So at least each search engine adapter must match generic test.
For sure you can add additionnal test in your implementation to test specific case (error catching....)
3 commits, have been done.
@lmignon @qgroulard
Note: on VCR we now match the raw_body so it avoid extra manual useless code that check that.