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

MFTF / Remove redundant ActionGroups #21647

Merged
merged 4 commits into from
Apr 8, 2019
Merged

MFTF / Remove redundant ActionGroups #21647

merged 4 commits into from
Apr 8, 2019

Conversation

lbajsarowicz
Copy link
Contributor

@lbajsarowicz lbajsarowicz commented Mar 8, 2019

Description (*)

I've removed files that differ from each other by capitalisation:

  • Removed 'app/code/Magento/Customer/Test/Mftf/ActionGroup/AssertStoreFrontPasswordAutocompleteOffActionGroup.xml'
  • Left 'app/code/Magento/Customer/Test/Mftf/ActionGroup/AssertStorefrontPasswordAutocompleteOffActionGroup.xml'
  • Removed 'app/code/Magento/Customer/Test/Mftf/ActionGroup/StoreFrontClickSignInButtonActionGroup.xml'
  • Left 'app/code/Magento/Customer/Test/Mftf/ActionGroup/StorefrontClickSignInButtonActionGroup.xml'

Fixed Issues (if relevant)

  • No issues created for that change

Manual testing scenarios (*)

  1. Validated manually if these ActionGroups with uppercase StoreFront are not used anywhere:

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 on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @lbajsarowicz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Mar 8, 2019

#magettestfest19

To avoid creating multiple PRs for single set of changes, I've added extra fix for Catalog AdminChangeProductAttributeSet test

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Mar 11, 2019

CLA assistant check
All committers have signed the CLA.

@rogyar rogyar self-assigned this Mar 22, 2019
-->
<actionGroups xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/actionGroupSchema.xsd">
<actionGroup name="AssertStorefrontPasswordAutoCompleteOffActionGroup">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, take a look at app/code/Magento/Customer/Test/Mftf/Test/PasswordAutocompleteOffTest.xml:53. This action group is used within the scope of the mentioned test. Why it's considered as redundant?

Copy link
Contributor Author

@lbajsarowicz lbajsarowicz Mar 22, 2019

Choose a reason for hiding this comment

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

After reading the PR description you should have noticed that I'm removing duplicated files with different names. The one I'm removing contains StoreFront (uppercase F), the one used in PasswordAutocompleteOffTest is Storefront (lowercase f)

        <actionGroup ref="AssertStorefrontPasswordAutoCompleteOffActionGroup" stepKey="assertStorefrontPasswordAutoCompleteOff"/>


<actionGroups xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="urn:magento:mftf:Test/etc/actionGroupSchema.xsd">
<actionGroup name="StorefrontClickSignInButtonActionGroup">
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, take a look at app/code/Magento/Customer/Test/Mftf/Test/PasswordAutocompleteOffTest.xml:50. This action group is used within the scope of the mentioned test. Why it's considered as redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same situaiton. I'm removing StoreFront, you are mentioning use of Storefront 🤦‍♂️

@lbajsarowicz lbajsarowicz requested a review from rogyar March 22, 2019 10:34
@rogyar
Copy link
Contributor

rogyar commented Mar 22, 2019

For QA.
The duplicated redundant versions of files are removed within the scope of current PR. It might bring additional troubles if you use case-insensitive filesystem such as AFS. Upon pulling the new changes, all versions of files will be removed (because of GIT+case-insensitive fs particularity). However, if you clone a fresh version, this problem does not occur.
On production environments, in most cases, we have case-sensitive file system, so this change should not have a compatibility issue.
However, the following scenario is good to be tested on a Linux environment:

  • Add the fork repository to your existing Magento repository as an additional remote
  • Switch and pull the branch with the changes of current PR
  • Make sure that you have at least one version of the file: AssertStoreFrontPasswordAutocompleteOffActionGroup.xml or AssertStorefrontPasswordAutocompleteOffActionGroup.xml

@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-4556 has been created to process this Pull Request

@lbajsarowicz
Copy link
Contributor Author

I see that one more case-based issue was merged recently:

Cloning into 'm23-dev'...
remote: Enumerating objects: 2099704, done.
remote: Total 2099704 (delta 0), reused 0 (delta 0), pack-reused 2099704
Receiving objects: 100% (2099704/2099704), 496.95 MiB | 6.74 MiB/s, done.
Resolving deltas: 100% (1187902/1187902), done.
Checking out files: 100% (34280/34280), done.
warning: the following paths have collided (e.g. case-sensitive paths
on a case-insensitive filesystem) and only one from the same
colliding group is in the working tree:

  'app/code/Magento/Customer/Test/Mftf/ActionGroup/AssertStoreFrontPasswordAutocompleteOffActionGroup.xml'
  'app/code/Magento/Customer/Test/Mftf/ActionGroup/AssertStorefrontPasswordAutocompleteOffActionGroup.xml'
  'app/code/Magento/Customer/Test/Mftf/ActionGroup/StoreFrontClickSignInButtonActionGroup.xml'
  'app/code/Magento/Customer/Test/Mftf/ActionGroup/StorefrontClickSignInButtonActionGroup.xml'
  'app/code/Magento/UrlRewrite/Test/Mftf/MetaData/url_rewrite-meta.xml'
  'app/code/Magento/UrlRewrite/Test/Mftf/Metadata/url_rewrite-meta.xml'

Screenshot 2019-03-29 at 23 37 54

Screenshot 2019-03-29 at 23 38 22

So @rogyar I will add one more fix 🤦‍♂️ to introduced issue

@okolesnyk okolesnyk self-requested a review April 4, 2019 17:17
Copy link
Member

@okolesnyk okolesnyk left a comment

Choose a reason for hiding this comment

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

Update: Figured what you mean about StoreFront vs Storefront filename. removed my comments cause I was wrong. Thanks for it.

But we shouldn't delete createUrlRewrite operation cause we use it in test runtime.

@m2-assistant
Copy link

m2-assistant bot commented Apr 8, 2019

Hi @lbajsarowicz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

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.

6 participants