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

Fixes unstable email integration tests, by decoding the raw contents … #25296

Merged

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Oct 26, 2019

…which had a maximum line length of 76 to a parseable string without a maximum line length, so there is no chance a particular string used in an assertion is getting splitted over multiple lines.

Description (*)

This problem was discovered during the contribution day in Amsterdam during Magento Live EU 2019
Here are some failing tests which shouldn't fail: https://testing-service.magento-community.engineering/reports/magento/magento2/pull/25183/7ec3a4e1ee9c98d60a692831691dfa57/Integration/allure-report-ce/index.html (only in Magento\Newsletter\Model\SubscriberTest & Magento\ProductAlert\Model\ObserverTest) related to PR #25183

The problem here, is that the tests are trying to find a string (for example You have been successfully subscribed to our newsletter.) in the html of the email output. But the email output is Quoted-printable. One of the characteristics of this, is that all lines of the email should be maximum 76 characters long. This means that sometimes the string which is being searched for in the integration tests, can be splitted over multiple lines and thus the integration test to fail if some parts of the email get changed by an unrelated change which shifts the characters a bit to the left or the right and causing the searched string to get splitted over 2 lines. So these integration tests are quite unstable.

This PR fixes this.

There are actually at least 2 solutions which could work:

I've chosen the first one, but if it would be better to use the second one, or even another alternative, let me know!

/cc: @okorshenko, @Echron

Difference between the output:

Date: Sat, 26 Oct 2019 11:24:25 +0000
MIME-Version: 1.0
Content-Type: text/html;
 charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: =?utf-8?Q?inline?=
Subject: Newsletter subscription success
To: [email protected]
From: =?utf-8?Q?Owner?= <[email protected]>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.=
org/TR/xhtml1/DTD/xhtml1-strict.dtd">=0A<html xmlns=3D"http://www.w3.org=
/1999/xhtml" style=3D"font-size: 62.5%; -webkit-text-size-adjust: 100%;=20=
-ms-text-size-adjust: 100%; font-size-adjust: 100%; background-color: #f=
5f5f5;">=0A<head>=0A    <meta http-equiv=3D"Content-Type" content=3D"tex=
t/html; charset=3Dutf-8">=0A    <meta name=3D"viewport" content=3D"initi=
al-scale=3D1.0, width=3Ddevice-width">=0A    <meta http-equiv=3D"X-UA-Co=
mpatible" content=3D"IE=3Dedge">=0A    <style type=3D"text/css">=0A    =20=
   =0A=0A        @import url("http://localhost/pub/static/version1572088=
283/frontend/Magento/luma/en_US/css/email-fonts.css");=0Ahtml {=0A  font=
-size: 62.5%;=0A  -webkit-text-size-adjust: 100%;=0A  -ms-text-size-adju=
st: 100%;=0A  font-size-adjust: 100%;=0A}=0Abody {=0A  color: #333333;=
=0A  font-family: 'Open Sans', 'Helvetica Neue', Helvetica, Arial, sans-=
...

vs

Date: Sat, 26 Oct 2019 11:24:25 +0000
MIME-Version: 1.0
Content-Type: text/html;
 charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: =?utf-8?Q?inline?Subject: Newsletter subscription success
To: [email protected]
From: =?utf-8?Q?Owner?= <[email protected]>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" style="font-size: 62.5%; -webkit-text-size-adjust: 100%; -ms-text-size-adjust: 100%; font-size-adjust: 100%; background-color: #f5f5f5;">
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
    <meta name="viewport" content="initial-scale=1.0, width=device-width">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <style type="text/css">


        @import url("http://localhost/pub/static/version1572088283/frontend/Magento/luma/en_US/css/email-fonts.css");
html {
  font-size: 62.5%;
  -webkit-text-size-adjust: 100%;
  -ms-text-size-adjust: 100%;
  font-size-adjust: 100%;
}
body {
  color: #333333;
  font-family: 'Open Sans', 'Helvetica Neue', Helvetica, Arial, sans-serif;
...

Fixed Issues (if relevant)

None that I could find

Manual testing scenarios (*)

  1. Put the following code in Magento\Newsletter\Model\SubscriberTest::testConfirm before the assert line
        echo $transportBuilder->getSentMessage()->getRawMessage() . "\n\n";
        echo " ======================== \n\n";
        echo quoted_printable_decode($transportBuilder->getSentMessage()->getRawMessage()) . "\n\n";
  1. Run cd dev/tests/integration
  2. Run ../../../vendor/bin/phpunit ../../../dev/tests/integration/testsuite/Magento/Newsletter/Model/SubscriberTest.php
  3. Look at the difference and notice that the second one is much more stable to do assertions on

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 Oct 26, 2019

Hi @hostep. 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.

@hostep
Copy link
Contributor Author

hostep commented Oct 26, 2019

I've just noticed that this problem was most likely caused by #23649, where most integration tests got fixed by using $transportBuilder->getSentMessage()->getBody()->getParts()[0]->getRawContent() instead of quoted_printable_decode

Should I update my PR, so it's more consistent with those other replacements?

@ihor-sviziev
Copy link
Contributor

Hi @hostep,
yes, please update your PR with suggested changes

@ihor-sviziev ihor-sviziev self-assigned this Oct 29, 2019
…which had a maximum line length of 76 to a parseable string without a maximum line length, so there is no chance a particular string used in an assertion is getting splitted over multiple lines.
@hostep hostep force-pushed the fix-unstable-email-tests branch from 36f482a to 27e8916 Compare October 29, 2019 19:45
@hostep
Copy link
Contributor Author

hostep commented Oct 29, 2019

Hi @ihor-sviziev

Changed all occurrences I could find, including the one which was originally already using quoted_printable_decode.

There is one case left where it's still using getSentMessage()->getRawMessage(), but it should stay like that, since it's testing for the headers of the email, not for the body.

@ihor-sviziev
Copy link
Contributor

@magento run Functional Tests

@ihor-sviziev
Copy link
Contributor

Looks like functional tests failures not related to your changes.

@magento-engcom-team
Copy link
Contributor

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

@engcom-Alfa engcom-Alfa self-assigned this Oct 30, 2019
@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Oct 30, 2019
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

@m2-assistant
Copy link

m2-assistant bot commented Nov 4, 2019

Hi @hostep, 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.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Nov 4, 2019
@VladimirZaets VladimirZaets added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: test coverage and removed Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests labels Nov 18, 2019
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