ADHOC/replicant-packages_apps_Email
3
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

More tests for IMAP, clean up, and a few bug fixes.

Browse Source 
- A few new tests in ImapStoreUnitTests.
- Added TODOs to ImapStoreUnitTests (for mainly NO response handling)
- Renamed ImapStore.releaseConnection to poolConnection.
- Fixed a bug in getConnection where it'd return a closed connection.
- Now getConnection() hanles BYE response for NOOP correctly and treat the
  connection as closed.

Change-Id: I48e5b89049338f7d4f1ac77cd7ac7243945a9575
This commit is contained in:
Makoto Onuki 2010-05-18 17:22:28 -07:00
parent 2552b7b705
commit cbe4ae9291
2 changed files with 174 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

85
src/com/android/email/mail/store/ImapStore.java
View File

@ -61,11 +61,13 @@ import java.nio.charset.Charset;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Date;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.regex.Pattern;
import javax.net.ssl.SSLException;
@ -101,8 +103,8 @@ public class ImapStore extends Store {
private String mIdPhrase = null;
private static String sImapId = null;
private final LinkedList<ImapConnection> mConnections =
new LinkedList<ImapConnection>();
private final ConcurrentLinkedQueue<ImapConnection> mConnectionPool =
new ConcurrentLinkedQueue<ImapConnection>();
/**
* Charset used for converting folder names to and from UTF-7 as defined by RFC 3501.
@ -118,6 +120,14 @@ public class ImapStore extends Store {
*/
private HashMap<String, ImapFolder> mFolderCache = new HashMap<String, ImapFolder>();
/**
* Next tag to use. All connections associated to the same ImapStore instance share the same
* counter to make tests simpler.
* (Some of the tests involve multiple connections but only have a single counter to track the
* tag.)
*/
private final AtomicInteger mNextCommandTag = new AtomicInteger(0);
/**
* Static named constructor.
*/
@ -182,6 +192,10 @@ public class ImapStore extends Store {
}
}
/* package */ Collection<ImapConnection> getConnectionPoolForTest() {
return mConnectionPool;
}
/**
* For testing only. Injects a different root transport (it will be copied using
* newInstanceWithConfiguration() each time IMAP sets up a new channel). The transport
@ -389,7 +403,7 @@ public class ImapStore extends Store {
connection.close();
throw new MessagingException("Unable to get folder list.", ioe);
} finally {
releaseConnection(connection);
poolConnection(connection);
}
}
@ -406,30 +420,35 @@ public class ImapStore extends Store {
}
/**
* Gets a connection if one is available for reuse, or creates a new one if not.
* @return
* Gets a connection if one is available from the pool, or creates a new one if not.
*/
private ImapConnection getConnection() throws MessagingException {
synchronized (mConnections) {
ImapConnection connection = null;
while ((connection = mConnections.poll()) != null) {
try {
connection.executeSimpleCommand("NOOP");
break;
}
catch (IOException ioe) {
connection.close();
}
/* package */ ImapConnection getConnection() {
ImapConnection connection = null;
while ((connection = mConnectionPool.poll()) != null) {
try {
connection.executeSimpleCommand("NOOP");
break;
} catch (MessagingException e) {
// Fall through
} catch (IOException e) {
// Fall through
}
if (connection == null) {
connection = new ImapConnection();
}
return connection;
connection.close();
connection = null;
}
if (connection == null) {
connection = new ImapConnection();
}
return connection;
}
private void releaseConnection(ImapConnection connection) {
mConnections.offer(connection);
/**
* Save a {@link ImapConnection} in the pool for reuse.
*/
/* package */ void poolConnection(ImapConnection connection) {
if (connection != null) {
mConnectionPool.add(connection);
}
}
/* package */ static String encodeFolderName(String name) {
@ -559,7 +578,7 @@ public class ImapStore extends Store {
// TODO implement expunge
mMessageCount = -1;
synchronized (this) {
releaseConnection(mConnection);
poolConnection(mConnection);
mConnection = null;
}
}
@ -602,7 +621,7 @@ public class ImapStore extends Store {
}
finally {
if (mConnection == null) {
releaseConnection(connection);
poolConnection(connection);
}
}
}
@ -642,7 +661,7 @@ public class ImapStore extends Store {
}
finally {
if (mConnection == null) {
releaseConnection(connection);
poolConnection(connection);
}
}
}
@ -1326,7 +1345,6 @@ public class ImapStore extends Store {
private static final String IMAP_DEDACTED_LOG = "[IMAP command redacted]";
private Transport mTransport;
private ImapResponseParser mParser;
private int mNextCommandTag;
/** # of command/response lines to log upon crash. */
private static final int DISCOURSE_LOGGER_SIZE = 64;
private final DiscourseLogger mDiscourse = new DiscourseLogger(DISCOURSE_LOGGER_SIZE);
@ -1336,8 +1354,6 @@ public class ImapStore extends Store {
return;
}
mNextCommandTag = 1;
try {
// copy configuration into a clean transport, if necessary
if (mTransport == null) {
@ -1441,9 +1457,14 @@ public class ImapStore extends Store {
// }
if (mTransport != null) {
mTransport.close();
mTransport = null;
}
}
/* package */ boolean isTransportOpenForTest() {
return mTransport != null ? mTransport.isOpen() : false;
}
public ImapResponse readResponse() throws IOException, MessagingException {
return mParser.readResponse();
}
@ -1459,7 +1480,7 @@ public class ImapStore extends Store {
public String sendCommand(String command, boolean sensitive)
throws MessagingException, IOException {
open();
String tag = Integer.toString(mNextCommandTag++);
String tag = Integer.toString(mNextCommandTag.incrementAndGet());
String commandToSend = tag + " " + command;
mTransport.writeLine(commandToSend, sensitive ? IMAP_DEDACTED_LOG : null);
mDiscourse.addSentCommand(sensitive ? IMAP_DEDACTED_LOG : commandToSend);
@ -1467,12 +1488,12 @@ public class ImapStore extends Store {
}
public List<ImapResponse> executeSimpleCommand(String command) throws IOException,
ImapException, MessagingException {
MessagingException {
return executeSimpleCommand(command, false);
}
public List<ImapResponse> executeSimpleCommand(String command, boolean sensitive)
throws IOException, ImapException, MessagingException {
throws IOException, MessagingException {
String tag = sendCommand(command, sensitive);
ArrayList<ImapResponse> responses = new ArrayList<ImapResponse>();
ImapResponse response;

131
tests/src/com/android/email/mail/store/ImapStoreUnitTests.java
View File

@ -19,6 +19,7 @@ package com.android.email.mail.store;
import com.android.email.Email;
import com.android.email.Utility;
import com.android.email.mail.Address;
import com.android.email.mail.AuthenticationFailedException;
import com.android.email.mail.Body;
import com.android.email.mail.FetchProfile;
import com.android.email.mail.Flag;
@ -34,6 +35,7 @@ import com.android.email.mail.internet.MimeBodyPart;
import com.android.email.mail.internet.MimeMultipart;
import com.android.email.mail.internet.MimeUtility;
import com.android.email.mail.internet.TextBody;
import com.android.email.mail.store.ImapStore.ImapConnection;
import com.android.email.mail.store.ImapStore.ImapMessage;
import com.android.email.mail.transport.MockTransport;
@ -54,6 +56,8 @@ import java.util.HashMap;
* $ runtest -c com.android.email.mail.store.ImapStoreUnitTests email
*
* TODO Check if callback is really called
* TODO test for BAD response in various places?
* TODO test for BYE response in various places?
*/
@SmallTest
public class ImapStoreUnitTests extends AndroidTestCase {
@ -87,6 +91,7 @@ public class ImapStoreUnitTests extends AndroidTestCase {
mStore = (ImapStore) ImapStore.newInstance("imap://user:password@server:999",
getContext(), null);
mFolder = (ImapStore.ImapFolder) mStore.getFolder(FOLDER_NAME);
mNextTag = 1;
}
/**
@ -103,6 +108,17 @@ public class ImapStoreUnitTests extends AndroidTestCase {
// TODO: inject specific facts in the initial folder SELECT and check them here
}
public void testLoginFailure() throws Exception {
MockTransport mockTransport = openAndInjectMockTransport();
expectLogin(mockTransport, new String[] {"* ID NIL", "OK"}, "NO authentication failed");
try {
mStore.getConnection().open();
fail("Didn't throw AuthenticationFailedException");
} catch (AuthenticationFailedException expected) {
}
}
/**
* TODO: Test with SSL negotiation (faked)
* TODO: Test with SSL required but not supported
@ -384,19 +400,29 @@ public class ImapStoreUnitTests extends AndroidTestCase {
}
private void expectLogin(MockTransport mockTransport, String[] imapIdResponse) {
// Fix the tag # of the ID response
String last = imapIdResponse[imapIdResponse.length-1];
last = "2 " + last;
imapIdResponse[imapIdResponse.length-1] = last;
expectLogin(mockTransport, imapIdResponse, "OK user authenticated (Success)");
}
private void expectLogin(MockTransport mockTransport, String[] imapIdResponse,
String loginResponse) {
// inject boilerplate commands that match our typical login
mockTransport.expect(null, "* OK Imap 2000 Ready To Assist You");
mockTransport.expect("1 CAPABILITY", new String[] {
mockTransport.expect(getNextTag(false) + " CAPABILITY", new String[] {
"* CAPABILITY IMAP4rev1 STARTTLS AUTH=GSSAPI LOGINDISABLED",
"1 OK CAPABILITY completed"});
mockTransport.expect("2 ID \\(.*\\)", imapIdResponse);
mockTransport.expect("3 LOGIN user \"password\"",
"3 OK user authenticated (Success)");
mNextTag = 4;
getNextTag(true) + " OK CAPABILITY completed"});
String expectedNextTag = getNextTag(false);
// Fix the tag # of the ID response
String last = imapIdResponse[imapIdResponse.length-1];
last = expectedNextTag + " " + last;
imapIdResponse[imapIdResponse.length-1] = last;
mockTransport.expect(getNextTag(false) + " ID \\(.*\\)", imapIdResponse);
getNextTag(true); // Advance the tag for ID response.
mockTransport.expect(getNextTag(false) + " LOGIN user \"password\"",
getNextTag(true) + " " + loginResponse);
}
/**
@ -500,6 +526,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
assertEquals("multipart/mixed; boundary=a00000000000000000000000000b",
message.getHeader("Content-Type")[0]);
assertTrue(message.isSet(Flag.SEEN));
// TODO: Test NO response.
}
public void testFetchBodyStructure() throws MessagingException {
@ -595,6 +623,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
// TODO Test for quote: If a filename contains ", it should become %22,
// which isn't implemented.
// TODO: Test NO response.
}
public void testFetchBodySane() throws MessagingException {
@ -616,6 +646,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
});
mFolder.fetch(new Message[] { message }, fp, null);
assertEquals("a@b.com", message.getHeader("from")[0]);
// TODO: Test NO response.
}
public void testFetchBody() throws MessagingException {
@ -637,6 +669,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
});
mFolder.fetch(new Message[] { message }, fp, null);
assertEquals("a@b.com", message.getHeader("from")[0]);
// TODO: Test NO response.
}
public void testFetchAttachment() throws Exception {
@ -682,6 +716,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
assertEquals("abc",
Utility.fromUtf8(IOUtils.toByteArray(mimePart1.getBody().getInputStream())));
// TODO: Test NO response.
}
/**
@ -808,6 +844,9 @@ public class ImapStoreUnitTests extends AndroidTestCase {
assertEquals("13", message.getUid());
assertEquals(7, mFolder.getMessageCount());
// TODO: Test append failure
// TODO: Test OK without APPENDUID
}
public void testGetPersonalNamespaces() throws Exception {
@ -834,6 +873,7 @@ public class ImapStoreUnitTests extends AndroidTestCase {
);
// TODO test with path prefix
// TODO: Test NO response.
}
public void testEncodeFolderName() {
@ -852,6 +892,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
assertEquals("!\u65E5\u672C\u8A9E!", ImapStore.decodeFolderName("!&ZeVnLIqe-!"));
}
// TODO test folder open failure
public void testExists() throws Exception {
MockTransport mock = openAndInjectMockTransport();
expectLogin(mock, new String[] {"* ID NIL", "OK"});
@ -930,6 +972,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
});
mFolder.copyMessages(messages, folderTo, null);
// TODO: Test NO response. (src message not found)
}
public void testGetUnreadMessageCount() throws Exception {
@ -957,6 +1001,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
});
mFolder.expunge();
// TODO: Test NO response. (permission denied)
}
public void testSetFlags() throws Exception {
@ -984,6 +1030,8 @@ public class ImapStoreUnitTests extends AndroidTestCase {
getNextTag(true) + " OK success"
});
mFolder.setFlags(messages, new Flag[] {Flag.DELETED}, false);
// TODO: Test NO response. (src message not found)
}
public void testGetMessage() throws Exception {
@ -1078,4 +1126,67 @@ public class ImapStoreUnitTests extends AndroidTestCase {
}
MoreAsserts.assertEquals(expectedUids, list.toArray(new String[0]) );
}
/**
* Test for {@link ImapStore#getConnection} and {@link ImapStore#keepConnectionForReuse}
*/
public void testGetConnection() throws Exception {
MockTransport mock = openAndInjectMockTransport();
// Start: No pooled connections.
assertEquals(0, mStore.getConnectionPoolForTest().size());
// Get 1st connection.
final ImapConnection con1 = mStore.getConnection();
assertNotNull(con1);
assertEquals(0, mStore.getConnectionPoolForTest().size()); // Pool size not changed.
assertFalse(con1.isTransportOpenForTest()); // Transport not open yet.
// Open con1
expectLogin(mock, new String[] {"* ID NIL", "OK"});
con1.open();
assertTrue(con1.isTransportOpenForTest());
// Get 2nd connection.
final ImapConnection con2 = mStore.getConnection();
assertNotNull(con2);
assertEquals(0, mStore.getConnectionPoolForTest().size()); // Pool size not changed.
assertFalse(con2.isTransportOpenForTest()); // Transport not open yet.
// con1 != con2
assertNotSame(con1, con2);
// Open con2
expectLogin(mock, new String[] {"* ID NIL", "OK"});
con2.open();
assertTrue(con1.isTransportOpenForTest());
// Now we have two open connections: con1 and con2
// Save con1 in the pool.
mStore.poolConnection(con1);
assertEquals(1, mStore.getConnectionPoolForTest().size());
// Get another connection. Should get con1, after verifying the connection.
mock.expect(getNextTag(false) + " NOOP", new String[] {getNextTag(true) + " OK success"});
final ImapConnection con1b = mStore.getConnection();
assertEquals(0, mStore.getConnectionPoolForTest().size()); // No connections left in pool
assertSame(con1, con1b);
assertTrue(con1.isTransportOpenForTest()); // We opened it.
// Save con2.
mStore.poolConnection(con2);
assertEquals(1, mStore.getConnectionPoolForTest().size());
// Try to get connection, but this time, connection gets closed.
mock.expect(getNextTag(false) + " NOOP", new String[] {getNextTag(true) + "* BYE bye"});
final ImapConnection con3 = mStore.getConnection();
assertNotNull(con3);
assertEquals(0, mStore.getConnectionPoolForTest().size()); // No connections left in pool
// It should be a new connection.
assertNotSame(con1, con3);
assertNotSame(con2, con3);
}
}