656984 - Fakeserver fails to support multiple connections properly

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: First stabs

So, this is a deficiency that I only realized when I started running some IMAP tests under QAserver a while back but avoided fixing because it would likely require several test changes.

Right now, the fakeserver code uses the same handler for all connections, which is fine if your connection is more or less stateless (POP, SMTP), but is going to cause things to break if it's stateful (NNTP to a lesser degree, IMAP to a huge degree). The simplest thing to do is to more or less clone the handler, but JS cloning is easier said than done.

Basically, the simplest thing that could be done is to ask each handler to clone itself when we need a new connection. This may be necessary for the IMAP fakeserver, to allow us to conduct tests on what happens if we are notified of external changes to the same folder. The downside is that it makes getting "the" handler for a connection harder, especially since it seems tests have started using the handler instead of the daemon to store information (:-().

I'm attaching my first stab at this: I only worry about IMAP and NNTP servers for now, so all tests using POP and SMTP will fail. It also causes several IMAP tests to fail due to use of server._handler, which is going away in favor of server._handlers.

My two options are to create a pseudo-handler object which proxies to all current handlers (which might need to forward to not-yet-created handlers?), or to shunt most of the ._handler stuff to the daemon instead. Which may be preferable in general.

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: New approach

This takes a different approach. Instead of trying to clone handlers, the server gets passed the daemon (this solidifying the daemon/handler divide) and a function to create the handler. The downside is that this requires changing pretty much every time the server is constructed; the upside is that it is a cleaner approach.

With this patch, the local/, compose/, and news/ tests all pass (but not imap/); I've also run individually the other test folders.

Things that I want to do not in this patch:
1. Fix IMAP tests
2. Move passwords from handler to the daemon

I'm not certain about where stuff like "drop on STARTTLS" and the like should go. In the longer term, I want to move fakeserver into a separate process. This would probably entail proxying the daemon object as the IPC, but I'm not 100% sure what to do about the handlers...

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Newer patch

This brings me down to two failing IMAP tests, which appear to have more complicated needs for the handler.

Comment 3

[David :Bienvenu]

Attachment: fix test_imapFolderCopy.js

I'll look at the other failing imap test tomorrow.

Comment 4

[David :Bienvenu]

Attachment: cumulative patch that fixes both imap test failures

Comment 5

[Joshua Cranmer [:jcranmer]]

Attachment: Combine all fixes

And this has all of the fixes applied to it.

Comment 6

[David :Bienvenu]

I'd recommend a try server push - Joshua, do you have try server access?

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 533341 [details] [diff] [review]
Combine all fixes

This generally looks good, though the unit tests are currently failing:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=13416167abb7

Do you also have an example test that uses this? From what I could tell, there's none that take advantage of it yet.

Comment 8

[Joshua Cranmer [:jcranmer]]

Attachment: Fixes all issues I'm aware of

This fixes the pop password failure test as well as adds a test to make sure that the connection stuff works properly. It's been pushed to try: <http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=f6e95bfc55fc>.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 554222 [details] [diff] [review]
Fixes all issues I'm aware of

Review of attachment 554222 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

Comment 10

[Joshua Cranmer [:jcranmer]]

Checked in to comm-central.