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

Make log mailer for testing #5893

Merged
merged 9 commits into from Feb 3, 2019

Conversation

7 participants
@zeripath
Copy link
Contributor

zeripath commented Jan 29, 2019

Multiple users have noticed in problems in 1.7.0 with issues to do with sending email. This is because there are code paths to do with emailing that are not tested in the integrations test due to gitea requiring a valid email configuration.

This PR creates a simple log mailer that can be used to test what kind of emails gitea would send.

Of note cherry-picking this and placing it on the v1.7 release master reveals the underlying bug in #5891.

zeripath added some commits Jan 29, 2019

Create log mailer for testing email settings
Signed-off-by: Andrew Thornton <art27@cantab.net>
Switch on the log mailer for the integration tests
This ensures that the sending mail process works

Signed-off-by: Andrew Thornton <art27@cantab.net>

@zeripath zeripath added the kind/build label Jan 29, 2019

@techknowlogick techknowlogick added this to the 1.8.0 milestone Jan 29, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Jan 29, 2019

Shortened log after cherry-picking this and placing it on releases/v1.7:

[Macaron] 2019-01-29 20:58:44: Started POST /api/v1/repos/user2/repo1/issues/1/comments?token=8a500d363a99c101fe1be475886b62fc0712947c for 
[Macaron] PANIC: runtime error: invalid memory address or nil pointer dereference
/usr/lib/go-1.10/src/runtime/panic.go:502 (0x438968)
	gopanic: reflectcall(nil, unsafe.Pointer(d.fn), deferArgs(d), uint32(d.siz), uint32(d.siz))
/usr/lib/go-1.10/src/runtime/panic.go:63 (0x4377fd)
	panicmem: panic(memoryError)
/usr/lib/go-1.10/src/runtime/signal_unix.go:388 (0x44e519)
	sigpanic: panicmem()
/home/andrew/go/src/code.gitea.io/gitea/models/issue_mail.go:81 (0xe55e31)
	mailIssueCommentToParticipants: if participants[i].ID == doer.ID {
/home/andrew/go/src/code.gitea.io/gitea/models/issue_comment.go:378 (0xe45d3c)
	(*Comment).MailParticipants: if err = mailIssueCommentToParticipants(e, issue, c.Poster, content, c, mentions); err != nil {
/home/andrew/go/src/code.gitea.io/gitea/models/issue_comment.go:635 (0xe4788f)
	sendCreateCommentAction: if err = comment.MailParticipants(e, act.OpType, opts.Issue); err != nil {
/home/andrew/go/src/code.gitea.io/gitea/models/issue_comment.go:533 (0xe46e2a)
	createComment: if err = sendCreateCommentAction(e, opts, comment); err != nil {
/home/andrew/go/src/code.gitea.io/gitea/models/issue_comment.go:814 (0xe48ac3)
	CreateComment: comment, err = createComment(sess, opts)
/home/andrew/go/src/code.gitea.io/gitea/models/issue_comment.go:831 (0xe48ca0)
	CreateIssueComment: comment, err := CreateComment(&CreateCommentOptions{
/home/andrew/go/src/code.gitea.io/gitea/routers/api/v1/repo/issue_comment.go:172 (0x1074fb0)

Revealing immediately the bug described in #5891

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #5893 into master will increase coverage by 0.24%.
The diff coverage is 34.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5893      +/-   ##
==========================================
+ Coverage   37.98%   38.22%   +0.24%     
==========================================
  Files         329      329              
  Lines       48406    48423      +17     
==========================================
+ Hits        18385    18508     +123     
+ Misses      27379    27249     -130     
- Partials     2642     2666      +24
Impacted Files Coverage Δ
modules/setting/setting.go 51.71% <10%> (+2.34%) ⬆️
modules/mailer/mailer.go 22.04% <53.84%> (+18.08%) ⬆️
models/repo_list.go 63.29% <0%> (-1.27%) ⬇️
routers/user/auth.go 13.28% <0%> (ø) ⬆️
models/issue.go 47.59% <0%> (+0.25%) ⬆️
modules/markup/markup.go 79.03% <0%> (+9.67%) ⬆️
models/mail.go 24.13% <0%> (+22.41%) ⬆️
models/issue_mail.go 53.84% <0%> (+42.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67567ef...34ff555. Read the comment docs.

@lafriks

This comment has been minimized.

Copy link
Member

lafriks commented Jan 30, 2019

I would prefer that inbucket service would be added to drone builds and that be used as SMTP server. Than no new fake tyoe would not be needed and later we could use it's api to check if message is actually sent

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Jan 30, 2019

Ok, I can understand that. I did wonder if we should make an internal channeled debug mailer myself - one that would just create a list of mail objects sent so you could query it in integration tests.

There may still be a point to having this sort of log mailer though. You can have it set on your development machine and test what is going to come out of Gitea without having to set up other servers. (Although you can kind of do this with the sendmail interface this would be built in unless we wanted to provide an appropriate sendmail proxy script.)

In any case, the point was to spur us to sort this out. We went live with a fairly major version and server 500s happened as soon as people turned on email notifications.

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Jan 31, 2019

Maybe we should name it dummy or testing but not a log because it only is used for testing. And that could be saved on a memory map or channel to confirm the testing but not on a log file.

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Feb 2, 2019

@techknowlogick

This comment has been minimized.

Copy link
Member

techknowlogick commented Feb 2, 2019

This PR looks great! I'm happy to give my 👍, but I like @lunny's idea of changing the name to testing, dummy, or something like that.

zeripath added some commits Feb 2, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor Author

zeripath commented Feb 2, 2019

Hi @techknowlogick Have changed it to dummy which I think is a good representation of what it is. We can make a testing one at some point which could be channel based. But at least with this we can test the emailing pathways.

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Feb 3, 2019

@techknowlogick techknowlogick merged commit 3d91bb2 into go-gitea:master Feb 3, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment