-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
[WebApi test] Disallow setting extension attributes as data array #27338
[WebApi test] Disallow setting extension attributes as data array #27338
Conversation
Hi @amenk. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
@amenk I assume we can close this PR? |
I have to closer check the results. Actually it seems to be a valid solution to the problem in the linked issue. |
Looks like valid solution. |
I have no clue why functional tests fail for B2B and EE -> rerunning |
Functional tests EE is green now without any change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @amenk ,
Your changes looks good to me. Could you cover them with any type of tests in order to prevent appearing this issue in future?
I think API test there is the most suitable.
Note: failing functional tests seems to me as false positive, no need to fix them
@ihor-sviziev I might not have the time in the near future to cover this with tests, I am happy if someone can take over. |
@ihor-sviziev can be related to this Pull Request |
@engcom-Kilo good test example! Thank you. |
@engcom-Kilo thank you |
@amenk as I understood - issue appears when you have extension attribute in the request, not in the model they you loaded. In this case it seems to me that declaring attribute is not mandatory. Let me know if I’m wrong |
unfortunately not,
Test does not fail, even my fix is not in place |
Also we better would make the test for a registered user... |
@magento run all tests |
@engcom-Kilo how come earlier these files were needed and now - no? |
Checked again, These sample files only for app/code/ . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔ Approved.
Failing tests looks not related to changes form this PR.
Hi @ihor-sviziev, thank you for the review. |
QA not applicable |
Hi @amenk, thank you for your contribution! |
Fixes #26682
While working on fix for the issue #26682 - fix already was delivered in fcd1b58#diff-e2b9d1a5f4b9f3503a2f8dd072bc2d8aR360, but it contains only unit tests, while this PR contained also web api tests, that better fits for such cases.
We decided to revert all changes related to the fix and deliver only tests as part of this PR.