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.

Sign up for GitHub

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

Jump to bottom

Compare arrays recursively fix #234

Merged
merged 4 commits into from
Aug 25, 2020
Merged

Compare arrays recursively fix #234

merged 4 commits into from
Aug 25, 2020

Conversation

bricht
Copy link
Contributor

@bricht bricht commented Aug 17, 2020 •
edited by le0n4ik
Loading

Description (*)

This is a fix to our branch to match the changes in magento/magento2#29458
Original suggestion was to override WebApiAbstract.php , but a separate class for this functionality was suggested by core team. In order to avoid redundant code later, i added this pull request.

Once that pr is merged in core and then in catalog-storefront we can remove the override.

Related Pull Requests

magento/magento2#29733
https://github.com/magento/partners-magento2ee/pull/312

Fixed Issues (if relevant)

  1. magento/partners-magento2ee#<issue_number>: Issue title

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

jekabs added 2 commits August 17, 2020 15:57
compare_arrays_recursively_fix
1d43cca 
-Removed compare arrays recursively from WebApiAbstract and added it in a separate method
compare_arrays_recursively_fix
e1414fe
le0n4ik
le0n4ik reviewed Aug 17, 2020
View reviewed changes
Copy link
Contributor

@le0n4ik le0n4ik left a comment

Choose a reason for hiding this comment

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

@mslabko If we are using this code in one test only (even if it abstract). Why we need this code in framework? May be it's better to move it to class where it used?

le0n4ik
le0n4ik suggested changes Aug 17, 2020
View reviewed changes
dev/tests/api-functional/framework/Magento/TestFramework/Helper/CompareArraysRecursively.php Outdated

/**
* Class for comparing arrays recursively
* TODO: This override can be removed once the same code is merged in magento core.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not an override. It's just a new class. Could you please remove TODO string?

Copy link
Contributor Author

@bricht bricht Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

By override I meant that its merged in magento core now. And when we have those changes we wont need it in catalog-storefront repo anymore. I changed the text in the todo to clarify this

dev/tests/api-functional/testsuite/Magento/GraphQl/Bundle/BundleProductMultipleOptionsTest.php Outdated
@@ -8,12 +8,28 @@
namespace Magento\GraphQl\Bundle;

use Magento\TestFramework\TestCase\GraphQlAbstract;
use Magento\TestFramework\Helper\Bootstrap;
use \Magento\TestFramework\Helper\CompareArraysRecursively;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove first "" symbol in class path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dome

dev/tests/api-functional/testsuite/Magento/GraphQl/Bundle/BundleProductMultipleOptionsTest.php Outdated
private $compareArraysRecursively;

/**
* @inheritDoc
Copy link
Contributor

Choose a reason for hiding this comment

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

use @inheritdoc (without camel case) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@bricht
Copy link
Contributor Author

bricht commented Aug 19, 2020

@mslabko If we are using this code in one test only (even if it abstract). Why we need this code in framework? May be it's better to move it to class where it used?

It was originally placed in GraphQlAbstract to compare deeply nested arrays. I wanted to use it outside GraphQl and it was useful for comparing two arrays where expected contains more keys/values than actual. In my case it was configurable product options.

@mslabko
Copy link
Member

mslabko commented Aug 20, 2020

@magento run WebAPI Tests, Static Tests

@mslabko
Merge remote-tracking branch 'origin/develop' into compare_arrays_rec…
55ff00b 
…ursively_fix

# Conflicts:
#	dev/tests/api-functional/testsuite/Magento/GraphQl/Bundle/BundleProductMultipleOptionsTest.php
@mslabko
Copy link
Member

mslabko commented Aug 20, 2020

@magento run WebAPI Tests, Static Tests

@mslabko
Copy link
Member

mslabko commented Aug 21, 2020

During sync with 24-develop, we make some mess and currently we have duplicates of compareArraysRecursively in 2.4-develop and broken functionality.

Some findings:

  1. we have duplicates of compareArraysRecursively in 2.4-develop https://github.com/magento/magento2/blob/2.4-develop/dev/tests/api-functional/framework/Magento/TestFramework/Helper/CompareArraysRecursively.php and https://github.com/magento/magento2/blob/2.4-develop/dev/tests/api-functional/framework/Magento/TestFramework/TestCase/GraphQlAbstract.php
  2. In storefront we override GraphQlAbstract and don't have this method > lead to error

Need to do:

  • eliminate duplicates in mainline
  • deliver this PR with removing deleted file

Propose:

  • move GraphQlAbstract::compareArraysRecursively to WebapiAbstract::compareArraysRecursively
  • reuse Magento/TestFramework/Helper/CompareArraysRecursively in WebapiAbstract::compareArraysRecursively

@le0n4ik le0n4ik mentioned this pull request Aug 24, 2020
Merged
4 tasks
@le0n4ik
Copy link
Contributor

le0n4ik commented Aug 24, 2020

@magento run all tests

@mslabko mslabko merged commit cf2beff into develop Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@le0n4ik le0n4ik le0n4ik requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bricht @mslabko @le0n4ik