Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit Test and Bug Fix for Issue 4 #14

Merged
merged 6 commits into from
Oct 15, 2014
Merged

Unit Test and Bug Fix for Issue 4 #14

merged 6 commits into from
Oct 15, 2014

Conversation

buildscientist
Copy link
Member

Submitting a PR containing the following changes:

  • A unit test confirming issue GreenMail IMAP does not escape quotes in ENVELOPE Message-ID response #4
  • An update to the JavaMail dependency (latest version is 1.5.2 - we're using 1.4.7)
  • An additional helper method in GreenMailUtil to accept a MimeMessage object to send to GreenMail SMTP
  • An inner class in GreenMailUtilTest (should probably be relocated to a test helper class) that extends MimeMessage and sets the message-id. JavaMail by default will set the message-id for each message it sends. See the following JavaMail FAQ for more details

youssuf.elkalay added 4 commits October 11, 2014 18:21
Set Javamail dependency version to 1.5.2 - latest version of Javamail as of 10/2014.
…e constructed emails to be sent through GreenMail

Added a test to GreenMailUtilTest to verify a bad envelope being generated when IMAP messages with message-id's that have unescaped characters are fetched. See #4 for more details.
…elines specified in #9

Modified JavaMail dependency in greenmail-parent POM and greenmail-core POM as requested in #9
…eld of a fetched email prior to it being added to a mail enevelope.

Added dependency on Apache Commons Lang to use a helper/utility class that escapes Java strings
@buildscientist buildscientist changed the title Issue 4 Unit Test and Bug Fix for Issue 4 Oct 14, 2014
@buildscientist buildscientist changed the title Unit Test and Bug Fix for Issue 4 Unit Test and Bug Fix for https://github.com/greenmail-mail-test/greenmail/issues/4 Oct 14, 2014
@buildscientist buildscientist changed the title Unit Test and Bug Fix for https://github.com/greenmail-mail-test/greenmail/issues/4 Unit Test and Bug Fix for Issue 4 Oct 14, 2014
…nt originally set and found in the retrieved email
<version>1.4.7</version>
<groupId>com.sun.mail</groupId>
<artifactId>javax.mail</artifactId>
<version>1.5.2</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the version here, it's already defined in the parent pom

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm a little confused - isn't this POM the parent? This is the one at the root of the project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you're right

@camann9
Copy link
Contributor

camann9 commented Oct 14, 2014

This time there are much less changes, that's good. Sadly we still have a lot of "phantom" changes (whitespace changes) that result form code formatter differences. We should have some code formatting guidelines.Eclipse can export the code formatter settings so that would be a solution.
One thing that is important though is not to format the whole file if you only change one line in a file. Then this kind of issues doesn't arise.

<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<exclusions>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this exclusion from here as it's already being defined in the parent POM.

@@ -363,6 +364,7 @@ String parseEnvelope() {
}
response.add(SP);
if (messageID != null && messageID.length > 0) {
messageID[0] = StringEscapeUtils.escapeJava(messageID[0]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you comment this line out and run the unit test here the test will fail.

@buildscientist
Copy link
Member Author

I'll make an effort not to format the entire file when making changes. Sorry - bad habit I suppose. I appreciate your patient with all this.

@camann9
Copy link
Contributor

camann9 commented Oct 15, 2014

No problem :-) . Thanks for your pull request.
I think having apache commons is a good thing.
I don't know if you're interested but if you want to fix more bugs go ahead :-)

camann9 added a commit that referenced this pull request Oct 15, 2014
Unit Test and Bug Fix for Issue 4
@camann9 camann9 merged commit 67be10f into greenmail-mail-test:master Oct 15, 2014
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants