The default bug view has changed. See this FAQ.

Fakeserver fails to support multiple connections properly

RESOLVED FIXED in Thunderbird 9.0

Status

MailNews Core
Testing Infrastructure
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jcranmer, Assigned: jcranmer)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 9.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 532309 [details] [diff] [review]
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.
Attachment #532309 - Flags: feedback?(mbanner)
Attachment #532309 - Flags: feedback?(dbienvenu)
(Assignee)

Comment 1

6 years ago
Created attachment 533083 [details] [diff] [review]
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...
Attachment #532309 - Attachment is obsolete: true
Attachment #532309 - Flags: feedback?(mbanner)
Attachment #532309 - Flags: feedback?(dbienvenu)
Attachment #533083 - Flags: feedback?(dbienvenu)
(Assignee)

Comment 2

6 years ago
Created attachment 533089 [details] [diff] [review]
Newer patch

This brings me down to two failing IMAP tests, which appear to have more complicated needs for the handler.
Attachment #533083 - Attachment is obsolete: true
Attachment #533083 - Flags: feedback?(dbienvenu)

Comment 3

6 years ago
Created attachment 533174 [details] [diff] [review]
fix test_imapFolderCopy.js

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

Comment 4

6 years ago
Created attachment 533278 [details] [diff] [review]
cumulative patch that fixes both imap test failures
Attachment #533174 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
Created attachment 533341 [details] [diff] [review]
Combine all fixes

And this has all of the fixes applied to it.
Attachment #533089 - Attachment is obsolete: true
Attachment #533278 - Attachment is obsolete: true
Attachment #533341 - Flags: review?(mbanner)

Comment 6

6 years ago
I'd recommend a try server push - Joshua, do you have try server access?
(Assignee)

Updated

6 years ago
Blocks: 662176
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.
Attachment #533341 - Flags: review?(mbanner) → review-
(Assignee)

Comment 8

6 years ago
Created attachment 554222 [details] [diff] [review]
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>.
Attachment #533341 - Attachment is obsolete: true
Attachment #554222 - Flags: review?(mbanner)
Comment on attachment 554222 [details] [diff] [review]
Fixes all issues I'm aware of

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

Looks good, thanks.
Attachment #554222 - Flags: review?(mbanner) → review+
(Assignee)

Comment 10

6 years ago
Checked in to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 9.0
You need to log in before you can comment on or make changes to this bug.