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: Extract Action Groups to separate files - magento/module-catalog #25854

Merged

Conversation

lbajsarowicz
Copy link
Contributor

Description (*)

Extract each Action Group to separate file, to follow MFTF Best Practices.

Fixed Issues (if relevant)

  1. Fix/inconsistent test names #22853

Questions or comments

Had to extract the changes per-module. The previous try failed, because of conflicts.

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)

@m2-assistant
Copy link

m2-assistant bot commented Nov 29, 2019

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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@lbajsarowicz
Copy link
Contributor Author

lbajsarowicz commented Dec 3, 2019

@okorshenko Are you really going to freeze these changes for another half year?

I have been aware that some of the name changings are backwards-incompatible, as the Action Group loader would not load files without ActionGroup suffix, so I had to append ActionGroup to the filename. That is kind of backward incompatibility that can be easily amended by our users with Replace All feature in their IDE and Regex pattern.

About freezing these PRs: That's not fair, actually because more and more people are onboarding to MFTF and depending on the crap that actually exists in the core and the sooner you introduce thorough cleanup that is actually made with my Pull Requests, the more QAs / Developers will come to the point that depending on anything in Magento is not worth the job.

We should introduce these changes ASAP, as 2.4 release is a matter of another year before release.
When I made a presentation during Mage UnConference, people were really glad to hear that finally there's cleanup made to the consistency of namings.

I've done that work before during Magento Imagine. You've been playing with me for 7 months so far, I've been merging latest 2.3-develop plenty of times, no one mentioned that my work is actually worthless.


I test my projects with MFTF and the changes I've made came from the need of having clean Action Groups pattern that when I'm looking for some action I'd like to perform - with PHPStorm I just click CTRL + SHIFT + N and type abstract of the action I want - like AddToCartAction and then look for the proper file.

Without my changes - you need to get through crappy files with even 51 of ActionGroups to find the addToCart that is completely against any rules or best practices for Magento Functional Testing Framework.

@torhoehn torhoehn self-assigned this Dec 4, 2019
@torhoehn torhoehn self-requested a review December 4, 2019 21:15
@magento-engcom-team
Copy link
Contributor

Hi @torhoehn, thank you for the review.
ENGCOM-6397 has been created to process this Pull Request
✳️ @torhoehn, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:09
@okorshenko
Copy link
Contributor

Hi @lbajsarowicz
If everything is fine with PR review, we will deliver it to 2.4 for sure. We want to have a clear convention for the upcoming release. Also we might need to modify XSD to cover this. Looking for @okolesnyk feedback.

In terms of 2.3 delivery, I think that we will be able to deliver it as well but following BC policy. As far as I know, you already implemented this solution. So thx for that! @okolesnyk is working with 2.3 release PO to get this prioritized. This change is useful and I don't think that it will be ignored in 2.3.

Thank you @lbajsarowicz for your involvement and moving this standard forward. We will do our best to get this delivered.

@magento-engcom-team
Copy link
Contributor

Hi @torhoehn, thank you for the review.
ENGCOM-6397 has been created to process this Pull Request
✳️ @torhoehn, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@dmytro-ch dmytro-ch added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: complex labels Dec 7, 2019
@dmytro-ch dmytro-ch self-assigned this Dec 7, 2019
@magento-engcom-team
Copy link
Contributor

Hi @dmytro-ch, thank you for the review.
ENGCOM-6397 has been created to process this Pull Request

@magento-engcom-team magento-engcom-team merged commit bb860cc into magento:2.4-develop Dec 20, 2019
@m2-assistant
Copy link

m2-assistant bot commented Dec 20, 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.

None yet

6 participants