REFACTOR: Use Async and family

[Ahmedgagan]

Hi @sj26 This PR is on behalf of Discourse. This PR replaces Eventmachine with Async.

[SamSaffron]

Howdy @sj26, anything we can do to help drive this forward? Do you need any assistance reviewing, I could ask another person from our team to carefully review.

[sj26]

Hi folks! This looks amazing, thanks for the contribution. I haven't had capacity to review this properly yet, and the changes are very extensive. It also looks like there are a few functional changes in here beyond refactoring which will need to be considered. I'll see if I can start dropping some smaller comments. (And I was leaving time for the "more changes" 🙂)

[Ahmedgagan]

Hi @sj26 Last week I made some commits as per your suggestions, Do have a look when you get time.

[sj26]

I've merged a switch to GitHub Actions to main so that there is some form of CI:

https://github.com/sj26/mailcatcher/pull/469

I'm not set on using mintest or even selenium (https://github.com/rubycdp/cuprite looks great!) but the fundamental infrastructure is available (ruby, node, chrome, etc).

[Ahmedgagan]

Hi @sj26 I have added a commit to convert Mintest to RSpec and All tests are passed(unless skipped) in my local. Starting to write SMTP implementation spec.

Also, If you wish to check Github CI setup works well with RSpec, Just approve this PR and GitHub actions will be performed on all my next commits

[Ahmedgagan]

What's this about? The CI workflow already sets up chrome and chromedriver: https://github.com/sj26/mailcatcher/blob/4d426e83d0a21611aae19ad2878babb1d9d2e905/.github/workflows/ci.yml#L27-L28

They work great. I've seen failures, but I'm pretty sure they're race conditions in usage. Using capybara with better assertions with waits would make the tests more reliable.

Yes I know its already setup, but I was trying to set up the same as https://github.com/SeleniumHQ/selenium/tree/trunk/.github/actions/setup-chrome But doing this also fails, removed the new setup

[sj26]

Cool, if you focus on testing the SMTP implementation then I'll see if I can rework the current test suite to be more reliable?

[Ahmedgagan]

Cool, if you focus on testing the SMTP implementation then I'll see if I can rework the current test suite to be more reliable?

Sure, I'll focus on SMTP tests

[sj26]

I've fixed the flakey test failures on main by switching to capybara and waiting for the websocket to connect before delivering examples (#472)

Feel free to rebase onto main to remove the CI/rspec switching gear. It'd be nice to keep this PR focused on the async transition.

[Ahmedgagan]

Hi @sj26 , I have added RSpec for smtp.rb file, if possible do have a look

[sj26]

Thanks @Ahmedgagan. The formatting of that file is a bit random, tabs and spaces etc, but I'll peer between that for the moment.

I can see it's testing some of the plumbing of the line protocol stuff. I'm not so interested in that, and more interested in testing that we're doing correct SMTP protocol interactions. Like can a new connection EHLO and receive an expected response. Does the MAIL FROM: ... command parse the sender correctly, and return expected responses in various cases. Does the server enforce a correct ordering of EHLO, MAIL FROM, RCPT TO, DATA and error correctly when done out of order. Do the encodings work — does it support 7bit transfer correctly, and utf8. Because this is a completely novel SMTP implementation I think these details are important. [edit: testing the dot stuffing behaviour is super important too, the <CRLF>.<CRLF> behaviour in DATA]

By SMTP clients I do indeed mean testing it manually from different environments, not just ruby's Net::SMTP. Like does it work from Django, or from Laravel, or other environments where devs are likely to use it. I agree that releasing a beta is a good way to do that too, but I don't want to drop such a beta without doing at least a little bit of independent testing first.

I didn't test if (m)any clients use BDAT before implementing it. If we can achieve all the required client support without it then I would consider removing it to simplify the implementation and testing. I think I only added it while playing with an SMTP server implementation and exploring ways to ingest emails with full octet encodings which have historically been a pain in MailCatcher's side. [edit: see later comment, I think we should remove CHUNKING/BDAT.]

So, a roadmap to getting this merged:

1. I need to write some more feature specs for MailCatcher to test things like attachments, downloading parts, Clear, Quit, and search. They should continue to pass on this branch.
2. This branch needs to be ruby 3.0 compatible. I noticed there are some errors/timeouts on CI around async tasks and ruby 3.
3. We need tests for the SMTP implementation to ensure correct implementation of the commands, and to ensure sequencing works as expect with appropriate responses. Encodings should also be carefully tested.
4. We need to do some manual testing using various SMTP client libraries which are typically used in popular development environments.

[sj26]

• Net::SMTP is the ruby defacto
• SwiftMailer's ESMTP seems the popular PHP smtp client
• Django uses smtplib from Python's standard library
• Golang has a standard smtp package

Most of them seem to support 8BITMIME and SMTPUTF8.

None support CHUNKING (BDAT) so I think we should rip that out.

[Ahmedgagan]

Hi @sj26 I made it work with ruby 3.0, and also added some more spec to test SMTP protocol interactions. Do share your thoughts.

Now, I am starting with testing it in different frameworks.

[Ahmedgagan]

Hi @sj26 I tested the SMTP implementation in python, Ruby, Golang & PHP. It worked in all 4 perfectly, there was a small problem with python in which it was sending commands in lowercase & our switch case was comparing uppercase strings. This commit solves it. I tested with sending plain text, HTML, attachments, attaching images in mail content, etc.

[Ahmedgagan]

So, a roadmap to getting this merged:

1. I need to write some more feature specs for MailCatcher to test things like attachments, downloading parts, Clear, Quit, and search. They should continue to pass on this branch.
2. This branch needs to be ruby 3.0 compatible. I noticed there are some errors/timeouts on CI around async tasks and ruby 3.
3. We need tests for the SMTP implementation to ensure correct implementation of the commands, and to ensure sequencing works as expect with appropriate responses. Encodings should also be carefully tested.
4. We need to do some manual testing using various SMTP client libraries which are typically used in popular development environments.

Hi @sj26 I think I have completed points 2,3,4. Can you have a look?

[sj26]

Thanks for testing it across implementations @Ahmedgagan!

Sorry it's taken me a while to take a look. This isn't my day job, so I have to steal time where I can.

These specs look a little light to me. There's lots of functionality which is untested, and quite a few status codes which aren't exercised. I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

I did manage to write quite a few more feature specs last weekend, but I haven't pushed them up yet. I wanted to get 0.8.0 out and stable with the improvements which had been collecting on the main branch.

[Ahmedgagan]

Sorry it's taken me a while to take a look. This isn't my day job, so I have to steal time where I can.

ooh, thanks for the efforts 😅

These specs look a little light to me. There's lots of functionality which is untested, and quite a few status codes which aren't exercised. I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

I'll also try to write some more concrete tests.

I did manage to write quite a few more feature specs last weekend, but I haven't pushed them up yet. I wanted to get 0.8.0 out and stable with the improvements which had been collecting on the main branch.

That's great, after the release we will merge those stable changes in this branch.

[Ahmedgagan]

I'll try to dig in and offer some more concrete suggestions or add some commits this weekend.

Hi @sj26 How are you? Can you have a look at the PR?

[SamSaffron]

Howdy @sj26 hope you are doing well! Anything we can do to move the merge forward.

We already had multiple people on ARM macs test this and it appears to be working nicely!

[SamSaffron]

@sj26 any news? Do you need a hand with the gem, I can ask around if any of our senior devs is open to helping maintain

[sj26]

@Ahmedgagan @SamSaffron sorry folks, I haven't had capacity to come back and do the project managementy bits. The world is not kind at the moment. I love this work, but I want it to land nicely and remain maintainable.

I'd like some more feature tests to make sure it's not breaking anything fundamental that folks currently use (working on main, and continuing to work on this branch), more test coverage of the novel smtp implementation's edges to make sure it doesn't regress, and I'd like it to match the code style a little more (it's silly but I'd prefer to keep to double quotes like the existing code, for example).

I think there is also a depth of edges which need careful review. For example, I think the new quit implementation currently just stops the reactor cold instead of sending a quit message to clients and ending the reactor after current connections are closed and finished (which the eventmachine implementation used to do by pushing to the bus and then stopping on the next tick). It needs a careful comb through.

If you have capacity to do these things, or know folks who can, that would be grand. Otherwise I'll get to it when I can.