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

[cypress] Fail in case of SMTP problems with a clear message #43492

Open
wants to merge 2 commits into
base: 4.4-dev
Choose a base branch
from

Conversation

muhme
Copy link
Contributor

@muhme muhme commented May 19, 2024

Summary of Changes

Improved Cypress system test failure messages in case of problems with SMTP.

If the array mail[] is empty width length 0, the line
cy.wrap(mails).should('have.lengthOf', 1);
does not cause the test to stop, in this case, as the Cypress assertion inside the .then() has an asynchronous nature.

The effect is that the test run continues and the following access to the mail fails with with a somehow incomprehensible message e.g.
TypeError: Cannot read properties of undefined (reading 'body')

Replaced it with an expect statement, which immediately triggers an error, and added an individual message that the SNMP configuration may need to be checked. Added two more places.

Trouble with snmp-tester port configuration was seen mutiple times, e.g. on configuring docker or on Ubuntu you need to open firewall with ufw allow 1025. May also be related to #42515 and #42516. Was discussed with @laoneo to check SNMP at the moment the snmp-tester is initialized. But that's only half the battle, because the SNMP configuration must work on Node and in the PHP-code of the server. This is why an individual test error message is used with a reference to the SNMP settings. And overall, a test must fail immediately if a mail is expected and is not there.

Testing Instructions

  • before PR
    • run all system tests e.g. with npm run cypress:run to see they are running without errors
    • hack configuration.php with an unused, non-LISTEN port e.g. $smtpport = '6789'
    • run the five system test specs with smtp-tester usage:
      npm run cypress:run -- --spec 'tests/System/integration/administrator/components/com_config/Application.cy.js,tests/System/integration/site/components/com_users/Remind.cy.js,tests/System/integration/site/components/com_privacy/Request.cy.js,tests/System/integration/site/components/com_users/Reset.cy.js,tests/System/integration/api/com_contact/Contacts.cy.js'
    • see the ugly errors Cannot read properties of undefined
  • after PR
    • run the five system test specs again and see the nice errors ... you may need to check the SMTP configuration
    • run all system tests e.g. with npm run cypress:run (including the initial Installation step which overwrites configuration.php with correct snmpport) to see they are running without errors

Actual result BEFORE applying this Pull Request

system tests fail in case of trouble with errors like:

  1) Test in frontend that the privacy request view
       can submit an information request of type export without a menu item:
     TypeError: Cannot read properties of undefined (reading 'sender')
      at Context.eval (webpack://joomla/./tests/System/integration/site/components/com_privacy/Request.cy.js:16:23)

Expected result AFTER applying this Pull Request

system tests fail in case of trouble with errors like:

  1) Test in frontend that the privacy request view
       can submit an information request of type export without a menu item:
     AssertionError: There are not two mails, you may need to check the SMTP configuration: expected 0 to equal 2
      at Context.eval (webpack://joomla/./tests/System/integration/site/components/com_privacy/Request.cy.js:15:103)

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

Upmerge

  • after this PR is merged, please upmerge in 5.1-dev, 5.2-dev and 6.0-dev branches

What was already tested?

  • the PR was exhausted tested with the following steps:

    1. clone dev branch
      • on Mac and Windows applied 3 files from PR 43466 (as not merged so far)
      • on 6.0-dev applied 1 file from PR 43400 (as not upmerged so far)
      • install and run full system tests npm run run:cypress w/o errors 111 specs / 436 tests
    2. 'hacked' $smtpport = '6789' as unused non-LISTEN port and run five system tests specs with smtp-tester usage
      npm run cypress:run -- --spec 'tests/System/integration/administrator/components/com_config/Application.cy.js,tests/System/integration/site/components/com_users/Remind.cy.js,tests/System/integration/site/components/com_privacy/Request.cy.js,tests/System/integration/site/components/com_users/Reset.cy.js,tests/System/integration/api/com_contact/Contacts.cy.js'
      11 failing from 22 tests / 5 specs
      to see the ugly errors
    3. fetched this PR and run again the 5 specs to see the nice errors 11 failing from 22 tests / 5 specs
    4. run full system tests npm run run:cypress w/o errors (this is doing Web installation step first and overwrites configuration.php with snmpport 1025)
  • did these four steps on the following platforms:
    a) local with branch 4.4-dev on Intel macOS 14.4.1 / Apache / MySQL
    b) local with branch 4.4-dev on Windows 11 Pro / Laragon
    c) on docker based installation
    . with branch 4.4-dev 435 tests (w/o Installation step)
    . with branch 5.1-dev 440 tests (w/o Installation step)
    . with branch 5.2-dev 440 tests (w/o Installation step)
    . with branch 6.0-dev 440 tests (w/o Installation step)

  • ignored test failure api/com_menus/SiteMenuItems.cy.js - can create a site menu item
    Save failed with the following error: Joomla\\Component\\Menus\\Administrator\\Table\\MenuTable::_getNode(1, id) failed." for the moment, assuming it is a mutliple test run problem

  • ignored two more test failures for the moment on 6.0-dev as they were also failing before the PR

muhme added 2 commits May 19, 2024 09:52
If the array `mail[]` is empty width length 0, the line
	`cy.wrap(mails).should('have.lengthOf', 1);`
does not cause the test to stop in this case as the Cypress
assertion inside the `.then()` has an asynchronous nature.
The effect is that the test run continues and the following access
to the mail fails with with a somehow incomprehensible message e.g.
	`TypeError: Cannot read properties of undefined (reading 'body')`

Replaced it with an `expect` statement, which immediately triggers an error,
and added an individual message that the SNMP configuration may need to be checked.

Added two more places.
@brianteeman
Copy link
Contributor

From reading this my understanding is that you are saying all the cypress tests are sending mail using smtp and the tests that I reported as failing were because my test server was not configured to use smtp and the changes here are so that the failure message is clear that it is failing because of smtp errors. I hope I understood that correctly.

If I have then I would make changes to the cypress readme etc to make it clear that the tests require a working smtp server. This was obvioulsy not apparent before hence the bug reports and as Joomla would normaly be configured to use phpmail or sendmail or smtp it explains why we had the "it works for me on my server" scenario. Even with this PR I am sure it could happen again if this requirement was not documented more clearly.

@muhme
Copy link
Contributor Author

muhme commented May 20, 2024

thank you for your comment and questions

From reading this my understanding is that you are saying all the cypress tests are sending mail using smtp

yes, and in node there is running the module smtp-tester; and smtp-tester in running smtp-server. During the system tests, an embedded SMTP server is therefore running on node. This is used to receive and check the mails

the tests that I reported as failing were because my test server was not configured to use smtp and the changes here are so that the failure message is clear that it is failing because of smtp errors. I hope I understood that correctly.

it would be interesting if you repeat the tests as described in this PR in your environment, we can also do a screen session together, i am now a little familiar with Laragon env

If I have then I would make changes to the cypress readme etc to make it clear that the tests require a working smtp server.

no, a working SMTP server is not required for the system tests, as this is embedded in the system test suite. important is that a free, unused smtp_port is configured and be opened in firewall, if there is one

and yes, this needs to be documented, i will do that along with an architecture picture, a few words about smtp-tester and joomla-cypress and additional hints for working with the Cypress tests (ELECTRON_ENABLE_LOGGING=1, it.only(), defaultCommandTimeout) - just waiting for the last README.md PR to be merged

@laoneo
Copy link
Member

laoneo commented May 20, 2024

Thanks for the pr' it is looking good. The only issue I have is that the SMTP server is a hard requirement for the test suite. So if it is not working on startup no test running should start at all as the environment is not properly setup. So I'm for an additiional check in the SMTP server setup function if the server is reachable.

@muhme
Copy link
Contributor Author

muhme commented May 20, 2024

Thanks for the pr' it is looking good. The only issue I have is that the SMTP server is a hard requirement for the test suite. So if it is not working on startup no test running should start at all as the environment is not properly setup. So I'm for an additiional check in the SMTP server setup function if the server is reachable.

  • this test exists already in administration/com_config/Application.cy.js with can send a test mail
  • and if you use an port that is already in use, as me web server port 8888, the overall test suite fails already with:
Running:  Application.cy.js                                                               (1 of 1)
Your configFile threw an error from: cypress.config.js

We stopped running your tests because your config file crashed.

Error: listen EADDRINUSE: address already in use :::8888
    at Server.setupListenHandle [as _listen2] (node:net:1872:16)
    at listenInCluster (node:net:1920:12)
    at Server.listen (node:net:2008:7)
    at SMTPServer.listen (/Users/hlu/Desktop/no_backup/improve-smtp-handling/node_modules/smtp-server/lib/smtp-server.js:104:28)

In my opinion, that's adequate, ok?

@brianteeman
Copy link
Contributor

/me confused
IF administration/com_config/Application.cy.js runs successfully then why dont the other tests with smtp run successfully

@muhme
Copy link
Contributor Author

muhme commented May 20, 2024

/me confused IF administration/com_config/Application.cy.js runs successfully then why dont the other tests with smtp run successfully

If Application.cy.js runs successfully then you may have different configurations in cypress.config.js and configuration.php. As the test in Application.cy.js is the only one, that takes the configuration only from cypress.config.js and used this to fill the Joomla formular to test mail. All other tests are using PHP configuration.php. Could you please check configuration.php:

public $mailer = 'smtp';
public $smtphost = 'localhost'; # must be the same as smtp_host in cypress.config.js
public $smtpport = 1025; # must be the same as smtp_port in cypress.config.js

configuration.php is created by the first Installation.cy.js correctly with the values from cypress.config.js. But the read-only problem, fixed in 43465, prevented setting the configuration values in the past.

Good luck 🫰

@brianteeman
Copy link
Contributor

That was it!!

Once the configuration.php was set to
public $mailer = 'smtp';
public $smtphost = 'localhost';
public $smtpport = 1025;

Then the tests worked - without the need of this pull request.

Therefore I would suggest that this PR could be improved by adding that smtp configuration info to the readme

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

4 participants