Test message rendering #9460

pabzm · 2024-05-21T15:29:35Z

This adds an integration testing setup to check the rendering of individual emails. The idea is to grow this with a lot of "normal" and "interesting" messages, to have a solid base for fixing new rendering errors without breaking previous fixes (motivated by #9443). It would also help with possible refactoring.

Run ./tests/MessageRendering/run-tests.sh to run all tests in that directory. They are ignored when running the main test suite (for now).

The messages are served by a dovecot container from the Maildir in
tests/MessageRendering/src/maildir and identified by their Message-Id. This way one can drop a raw email into there, and immediately write a test against it.

This is not yet built into the GitHub workflows, and also has only a few messages and tests, because before putting more work into this I'd like to have some feedback.

@roundcube/webmail-developers What do you think about this?

pabzm · 2024-05-21T17:45:11Z

Sorry, it took me a moment to wrestle with phpstan, the cs-fixer and windows filename problems. It's ready now and I won't force-push anymore.

miaulalala

Looks good!

pabzm · 2024-05-24T11:53:07Z

Actually this conflicts with the work I did for #9465 (which only occured to me yesterday), so I'd not push this before that other topic was discussed.

program/actions/mail/show.php

alecpl · 2024-06-01T06:41:37Z

tests/MessageRendering/MessageRenderingTestCase.php

+     * to be placed as individual files in
+     * `tests/src/emails/test@example/Mail/cur/`.
+     */
+    protected function run_and_get_html_output_domxpath(string $msg_id): \DOMXPath


Use camel-case consistently.

Sorry for mixing styles. https://github.com/roundcube/roundcubemail/wiki/Dev-Guidelines#classes-functions-and-variables says: "No CamelCase", but the repo has some. Which way would you prefer?

Camel-case here as this is a new class in tests.

Ok. Do you want CameCase only in new tests or in all new code?

No necessarily in tests, but in utility classes like this for sure. I think we can do this in tests too, especially in new files where there's no old code in another style.

Ok, so we continue snake_case if the file already contains some, but if not we default to CamelCase?

Only in tests, or also in other code?

(Sorry for insisting, I want to understand thoroughly.)

alecpl · 2024-06-01T06:45:53Z

tests/MessageRendering/MessageRenderingTestCase.php

+        $rcmail = \rcmail::get_instance();
+        // We need to overwrite the storage object, else storage_init() just
+        // returns the cached one (which might be a StorageMock instance).
+        $mock_storage = $rcmail->storage = null;


Yeah, we should probably not initialize ActionTestCase in tests bootstrap, but do it differently so it does not leak to other tests.

Until we improve that, can we use this code as it is? As far as I can see it doesn't break other tests.

alecpl · 2024-06-01T06:48:30Z

tests/MessageRendering/MessageRenderingTestCase.php

+        $mock_storage = $rcmail->storage = null;
+        $rcmail->storage_init();
+        // Login our test user so we can fetch messages from the imap server.
+        $rcmail->login('test', 'pass', 'tls://localhost');


In Browser tests we're using a standalone imap server from Greenmail. Maybe that would be better approach than dovecot. However, I don't know whether Greenmail's mail parsing and imap commands provide good quality results.

The Browser tests can also use any imap server, we need only username/password in config. That means you'd have to append the test messages if not in INBOX.

n Browser tests we're using a standalone imap server from Greenmail. Maybe that would be better approach than dovecot

I'll have a look at it, thank you for the hint!

That means you'd have to append the test messages if not in INBOX.

I'm not sure I understand: If using Greenmail I'd have to append the messages? Or if using other folders?

Currently the code is only using the INBOX, because it focusses on the message's content rendering, not how listings are rendered. So then I don't need to append any messages, right?

With dovecot you just start it with the maildir directory. With other servers you can't do this, especially with remote ones. See tests/Browser/bootstrap.php for some useful code that already exists.

Thank you for the explanation!

Why do you think using Greenmail would be better? It is also "just" a standalone IMAP-Server, isn't it? Since we're not using Java, we cannot hook into it for our tests, as far as I understand.
With two standalone IMAP-Servers, wouldn't it be better to test against the one that is most used in live systems?

A developer should be able to easily run these tests on their PC. I think that using a self-contained file is better than a need to install dovecot in the system. Java will be required indeed, but I bet most people have it already.

On the other hand people might want to run tests with their own remote IMAP server.

This setup uses docker to run dovecot, you don't have to install it locally. Don't you think docker is an acceptable dependency for a test suite?

@alecpl Could you let me know if you think docker (or a similar container setup) is an acceptable dependency?

I don't really like to sudo-execute a binary downloaded from the internet, and would much prefer a container. We could still use greenmail, if you prefer that: https://hub.docker.com/r/greenmail/standalone

tests/MessageRendering/src/maildir/cur/attachment-gh9443.eml

alecpl · 2024-06-01T06:59:00Z

tests/MessageRendering/bootstrap.php

@@ -0,0 +1,44 @@
+<?php


I would probably put these tests in tests/Actions/Mail/EmailRendering because it's testing the actions. Of course they would have to be skipped if imap server is not running or not configured.

But I understand you wanted to separate it on start.

I'd like to postpone the integration until it works as we want it, ok?

We can't test this code if it exits.

Run ./tests/MessageRendering/run-tests.sh to run all tests in that directory, which are supposed to check the rendering of concrete messages. The messages are served by a dovecot container from the Maildir in `tests/MessageRendering/src/maildir` and identified by their `Message-Id`.

The files will be renamed back by dovecot, but then they won't hurt our unit test suite anymore. We'll have to come up with a good way to avoid git reporting a lot of filename changes, though.

To use another image, e.g. to test a different server implementation, or because the default image doesn't run on your platform, create a file called 'tests/MessageRendering/.env' and assign the variable ROUNDCUBE_TEST_IMAP_SERVER_IMAGE in it.

github-actions · 2024-11-21T00:15:13Z

@pabzm, @alecpl
🛎️ This PR has had no activity in two weeks.

pabzm force-pushed the test-message-rendering branch 4 times, most recently from 825c1a3 to 0ceb1e8 Compare May 21, 2024 17:12

pabzm marked this pull request as draft May 21, 2024 17:13

pabzm force-pushed the test-message-rendering branch from 0ceb1e8 to 8f34373 Compare May 21, 2024 17:17

pabzm marked this pull request as ready for review May 21, 2024 17:45

miaulalala approved these changes May 24, 2024

View reviewed changes

pabzm added the 3. to review label May 29, 2024

pabzm self-assigned this May 29, 2024

alecpl requested changes Jun 1, 2024

View reviewed changes

pabzm force-pushed the test-message-rendering branch from 4c0b172 to 0a5f089 Compare June 4, 2024 16:05

pabzm and others added 11 commits June 14, 2024 13:14

Exit only if not in TEST_MODE

4a79c8f

We can't test this code if it exits.

Prevent phpstan from reporting skipped test as error

acad198

Rename message files to fix unit tests on windows

4846095

The files will be renamed back by dovecot, but then they won't hurt our unit test suite anymore. We'll have to come up with a good way to avoid git reporting a lot of filename changes, though.

Exit script in a way that is mock'able

455d55a

Rename folder containing test messages

4d435e6

Use CamelCase in new test files

770f545

Fix moving test files to data/

2abf095

Append to output, don't overwrite it

873204b

Fix tests for code using sendExit()

e533e1d

pabzm force-pushed the test-message-rendering branch from 0a5f089 to 63c90ef Compare June 14, 2024 11:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test message rendering #9460

Test message rendering #9460

pabzm commented May 21, 2024 •

edited

Loading

pabzm commented May 21, 2024

miaulalala left a comment

pabzm commented May 24, 2024

alecpl Jun 1, 2024

pabzm Jun 3, 2024

alecpl Jun 3, 2024

pabzm Jun 4, 2024

alecpl Jun 4, 2024

pabzm Jun 4, 2024

alecpl Jun 1, 2024

pabzm Jun 3, 2024

alecpl Jun 1, 2024

pabzm Jun 3, 2024

alecpl Jun 3, 2024

pabzm Jun 4, 2024

alecpl Jun 4, 2024

pabzm Jun 4, 2024

pabzm Jun 20, 2024

alecpl Jun 1, 2024

pabzm Jun 3, 2024

github-actions bot commented Nov 21, 2024

Test message rendering #9460

Are you sure you want to change the base?

Test message rendering #9460

Conversation

pabzm commented May 21, 2024 • edited Loading

pabzm commented May 21, 2024

miaulalala left a comment

Choose a reason for hiding this comment

pabzm commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2024

pabzm commented May 21, 2024 •

edited

Loading