Skip to content

Tests for: magento/magento2#25585, magento/magento2#25248, magento/magento2#25058. #25853

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
Closed
wants to merge 6 commits into from
Closed

Tests for: magento/magento2#25585, magento/magento2#25248, magento/magento2#25058. #25853

wants to merge 6 commits into from

Conversation

p-bystritsky
Copy link
Contributor

@p-bystritsky p-bystritsky commented Nov 29, 2019 •
edited
Loading

Description (*)

Tests for: #25585, #25248, #25058.

Fixed Issues (if relevant)

Manual testing scenarios (*)

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)

@m2-assistant
Copy link

m2-assistant bot commented Nov 29, 2019

Hi @p-bystritsky. 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.

@p-bystritsky p-bystritsky changed the title Tests for: magento/magento2#25585, magento/magento2#25585, magento/magento2#25058. Tests for: magento/magento2#25585, magento/magento2#25248, magento/magento2#25058. Nov 29, 2019
@dmytro-ch dmytro-ch self-assigned this Nov 29, 2019
lbajsarowicz
lbajsarowicz suggested changes Nov 29, 2019
View reviewed changes
Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

All the repetitive tasks should be extracted from Tests into Action Groups.

The main recommendation is that the Test should contain mostly Action Groups (with small exceptions)

@ghost ghost assigned lbajsarowicz Nov 29, 2019
lbajsarowicz
lbajsarowicz reviewed Nov 29, 2019
View reviewed changes
app/code/Magento/Catalog/Test/Mftf/Test/ProductAttributeWithoutValueInCompareListTest.xml Outdated
Comment on lines 59 to 57
<amOnPage url="{{StorefrontProductComparePage.url}}" stepKey="navigateToComparePage"/>
<waitForPageLoad stepKey="waitForStorefrontProductComparePageLoad"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

And this one with StorefrontOpenAndCheckComparisionActionGroup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Action Group is not applicable from the Product Page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider looking for other, creating your own

lbajsarowicz
lbajsarowicz reviewed Nov 29, 2019
View reviewed changes
app/code/Magento/Catalog/Test/Mftf/Test/ProductAttributeWithoutValueInCompareListTest.xml Outdated
<!--Assert success message-->
<grabTextFrom selector="{{AdminProductMessagesSection.successMessage}}" stepKey="grabTextFromSuccessMessage"/>
<assertEquals expected='You added product $$createProductDefault.name$$ to the comparison list.' expectedType="string"
actual="($grabTextFromSuccessMessage)" stepKey="assertSuccessMessage"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

This section could be replaced - for example - with StorefrontAddProductToCompareActionGroup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

lbajsarowicz
lbajsarowicz reviewed Nov 29, 2019
View reviewed changes
app/code/Magento/Catalog/Test/Mftf/Test/ProductAttributeWithoutValueInCompareListTest.xml Outdated
<!--Assert success message-->
<grabTextFrom selector="{{AdminProductMessagesSection.successMessage}}" stepKey="grabTextFromSuccessMessageCustom"/>
<assertEquals expected='You added product $$createProductCustom.name$$ to the comparison list.' expectedType="string"
actual="($grabTextFromSuccessMessageCustom)" stepKey="assertSuccessMessageCustom"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

StorefrontAddProductToCompareActionGroup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

This was referenced Dec 2, 2019
Merged
Merged
Merged
@ghost ghost assigned VladimirZaets Dec 2, 2019
@p-bystritsky
Copy link
Contributor Author

@lbajsarowicz updated.

@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:09
lbajsarowicz
lbajsarowicz suggested changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

You still need to extract repetitive actions to ActionGroups.

@p-bystritsky
Copy link
Contributor Author

@lbajsarowicz I don't know what else can be extracted to action groups.

@magento-engcom-team
Copy link
Contributor

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

@engcom-Delta engcom-Delta self-assigned this Dec 27, 2019
@engcom-Delta
Copy link
Contributor

Notice: QA not applicable

magento-engcom-team pushed a commit that referenced this pull request Dec 29, 2019
@VladimirZaets
ENGCOM-6377: Tests for: #25585, #25248, #25058. #25853
4c67f22
@VladimirZaets
Copy link
Contributor

@p-bystritsky Thanks. The main code is merged. Tests were updated by myself.

@m2-assistant
Copy link

m2-assistant bot commented Dec 29, 2019

Hi @p-bystritsky, 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
Reviewers

@VladimirZaets VladimirZaets VladimirZaets approved these changes

@akaplya akaplya Awaiting requested review from akaplya

@buskamuza buskamuza Awaiting requested review from buskamuza

@melnikovi melnikovi Awaiting requested review from melnikovi

@dmytro-ch dmytro-ch Awaiting requested review from dmytro-ch

+1 more reviewer

@lbajsarowicz lbajsarowicz lbajsarowicz approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@lbajsarowicz lbajsarowicz

@VladimirZaets VladimirZaets

@dmytro-ch dmytro-ch

@engcom-Delta engcom-Delta

Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: MFTF test coverage Component: Catalog Component: ImportExport Component: Newsletter Progress: extended testing Release Line: 2.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@p-bystritsky @magento-engcom-team @engcom-Delta @VladimirZaets @lbajsarowicz @sidolov @dmytro-ch @engcom-Foxtrot