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

Test coverage for PR #27589 (E-mail templates) #27606

Merged
merged 6 commits into from
Apr 18, 2020
Merged

Test coverage for PR #27589 (E-mail templates) #27606

merged 6 commits into from
Apr 18, 2020

Conversation

lbajsarowicz
Copy link
Contributor

@lbajsarowicz lbajsarowicz commented Apr 5, 2020 •
edited by slavvka
Loading

Description (*)

Additional Integration Tests coverage for PR provided by @ptylek

Related Pull Requests

Fixed Issues (if relevant)

  1. Use the same greeting as in other Magento Customer emails #27589

Related issues

  1. Integration Tests: DOM Assertion class #27607

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)

@m2-assistant
Copy link

m2-assistant bot commented Apr 5, 2020

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.4-develop instance - deploy vanilla Magento instance

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

@ghost ghost added this to Pending Review in Pull Requests Dashboard Apr 5, 2020
@lbajsarowicz lbajsarowicz changed the title Test coverage for PR #27589 Test coverage for PR #27589 (E-mail templates) Apr 5, 2020
@lbajsarowicz lbajsarowicz mentioned this pull request Apr 5, 2020
Merged
4 tasks
naydav
naydav requested changes Apr 5, 2020
View reviewed changes
dev/tests/integration/testsuite/Magento/Customer/Controller/Account/EmailTemplateTest.php Outdated
$emailDom->loadHTML($messageContent);

$emailXpath = new \DOMXPath($emailDom);
$greeting = $emailXpath->query('//p[@class="greeting"]')->item(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like if '//p[@class="greeting"]' will not found then it will lead to a fatal error
"Call method item() on no-objetc"
Please check it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressing the issue.

dev/tests/integration/testsuite/Magento/Customer/Controller/Account/EmailTemplateTest.php
{
$messageContent = $this->getMessageRawContent($message);
$emailDom = new \DOMDocument();
$emailDom->loadHTML($messageContent);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can introduce new "assert class" and put in
https://github.com/magento/magento2/tree/2.4-develop/dev/tests/integration/framework/Magento/TestFramework
Assert namespace.

We achieve 2 goals:

  • working with DOM will be encapsulated ;
  • we can reuse the "assert" in the other testcases;

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to create separate ticket for that point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to create separate ticket for that point.

Please link this and a new ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#27607

@ghost ghost moved this from Pending Review to Changes Requested in Pull Requests Dashboard Apr 5, 2020
@ghost ghost assigned naydav Apr 5, 2020
@ghost ghost assigned lbajsarowicz Apr 5, 2020
@ghost ghost moved this from Changes Requested to Ready for Testing in Pull Requests Dashboard Apr 5, 2020
@magento-engcom-team
Copy link
Contributor

Hi @naydav, thank you for the review.
ENGCOM-7262 has been created to process this Pull Request
✳️ @naydav, 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

@lbajsarowicz lbajsarowicz linked an issue Apr 5, 2020 that may be closed by this pull request
Integration Tests: DOM Assertion class #27607
Closed
@lbajsarowicz lbajsarowicz mentioned this pull request Apr 5, 2020
Merged
5 tasks
@engcom-Alfa engcom-Alfa self-assigned this Apr 8, 2020
@engcom-Alfa engcom-Alfa moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Apr 8, 2020
@ghost ghost moved this from Changes Requested to Merge in Progress in Pull Requests Dashboard Apr 10, 2020
@slavvka slavvka moved this from Merge in Progress to On Hold in Pull Requests Dashboard Apr 10, 2020
@slavvka slavvka moved this from On Hold to Pending Review in Pull Requests Dashboard Apr 10, 2020
@lbajsarowicz
Copy link
Contributor Author

@slavvka Sorry, I haven't noticed your message. Sure I'm going to fix the failing issue.
Also, label is incorrect as original issue was fixed by other contributor, I just covered change with tests.

@lbajsarowicz
Copy link
Contributor Author

@slavvka Failures were not related.

@ghost ghost moved this from Pending Review to Ready for Testing in Pull Requests Dashboard Apr 17, 2020
@ghost ghost assigned slavvka Apr 17, 2020
@slavvka slavvka moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Apr 17, 2020
@slavvka slavvka moved this from Testing in Progress to Merge in Progress in Pull Requests Dashboard Apr 17, 2020
@magento-engcom-team
Copy link
Contributor

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

magento-engcom-team pushed a commit that referenced this pull request Apr 18, 2020
@slavvka
ENGCOM-7262: Test coverage for PR #27589 (E-mail templates) #27606
5f57f06
@magento-engcom-team magento-engcom-team merged commit 45706d6 into magento:2.4-develop Apr 18, 2020
@m2-assistant
Copy link

m2-assistant bot commented Apr 18, 2020

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.

@ghost ghost moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Apr 18, 2020
@lbajsarowicz lbajsarowicz mentioned this pull request Apr 18, 2020
Merged
4 tasks
@magento-engcom-team magento-engcom-team mentioned this pull request May 1, 2020
Closed
@magento-engcom-team magento-engcom-team removed this from Recently Merged in Pull Requests Dashboard May 6, 2020
@sdzhepa sdzhepa mentioned this pull request May 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavvka slavvka slavvka approved these changes

@naydav naydav Awaiting requested review from naydav

Assignees

@naydav naydav

@lbajsarowicz lbajsarowicz

@slavvka slavvka

@engcom-Alfa engcom-Alfa

@engcom-Echo engcom-Echo

Labels
Area: Frontend Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Component: Customer Event: Global-Contribution-Day Partner: Mediotype partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

Integration Tests: DOM Assertion class
8 participants
@lbajsarowicz @magento-engcom-team @engcom-Alfa @engcom-Echo @slavvka @naydav @VladimirZaets @ptylek