Skip to content

chore(coderd/notifications): expand golden file testing for notifications #15032

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

Merged
merged 49 commits into from
Oct 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
70f3486
feat(notifications): Improve notification format consistency
SasSwart Oct 3, 2024
7e31a34
chore(coderd/notifications): regenerate notification testdata from th…
SasSwart Oct 5, 2024
cf3afd4
Merge remote-tracking branch 'origin/main' into jjs/consistent-notifi…
SasSwart Oct 5, 2024
4b85f2b
chore(coderd/database): renumber migration
SasSwart Oct 5, 2024
e8ad3ac
chore(coderd/notifications): regenerate testdata
SasSwart Oct 5, 2024
2a4d740
fix(coderd/notifications): remove duplicate function signature
SasSwart Oct 5, 2024
e741c43
chore: remove redundant escaping in migration
SasSwart Oct 5, 2024
adffe60
chore(coderd/notifications): improve failed test feedback
SasSwart Oct 5, 2024
41ed54a
feat(coderd/database): add new information to the account activated n…
SasSwart Oct 7, 2024
5541331
Merge remote-tracking branch 'origin/main' into jjs/additional-notifi…
SasSwart Oct 8, 2024
fe94f0d
chore(coderd/database): rework migration for legibility
SasSwart Oct 8, 2024
98e7501
feat(coderd): send newly required information to notification templates
SasSwart Oct 8, 2024
d8e00c2
feat(coderd/notifications): provide additional context to workspace n…
SasSwart Oct 8, 2024
d6a339f
fix(coderd/notifications): add a missing call to fmt.Sprintf
SasSwart Oct 8, 2024
920ad31
fix(coderd/notifications): fix oversights in template migration
SasSwart Oct 9, 2024
9e938e5
chore(coderd/provisionerdserver): set the displayname in TestNotifica…
SasSwart Oct 9, 2024
59e57ac
chore(coderd): add more robust testing assertions to TestNotifyDelete…
SasSwart Oct 9, 2024
c907238
chore(coderd/notifications): fix migration indentation
SasSwart Oct 9, 2024
2493556
chore(coderd/notifications): regenerate golden files
SasSwart Oct 9, 2024
fc18142
Merge remote-tracking branch 'origin/main' into jjs/consistent-notifi…
SasSwart Oct 9, 2024
eccef1c
chore(coderd/notifications): expand golden file testing for notificat…
SasSwart Oct 9, 2024
0036006
chore(coderd/notifications): ensure that notification golden files ar…
SasSwart Oct 9, 2024
e9eebf3
chore(coderd/notifications): remove defunct test assertions
SasSwart Oct 9, 2024
78f2787
chore(coderd/notifications): lint notifications_test
SasSwart Oct 9, 2024
aa7658b
Merge remote-tracking branch 'origin/main' into jjs/14913
SasSwart Oct 10, 2024
02535bb
feat(coderd/notifications): demonstrate the concept of smtp golden files
SasSwart Oct 10, 2024
7e013d8
chore(coderd/notifications): move mock smtp server into its own package
SasSwart Oct 10, 2024
a3b240d
chore(coderd/notifications): new golden files for smtp and wehbook
SasSwart Oct 11, 2024
8989a44
chore(coderd/notifications): replace sleep with require.eventually in…
SasSwart Oct 11, 2024
ff18208
Merge remote-tracking branch 'origin/main' into jjs/14913
SasSwart Oct 11, 2024
ed91226
chore(coderd/notifications): rename and regenerate notifications files
SasSwart Oct 11, 2024
91e3f94
chore(coderd/notifications): remove defunct test
SasSwart Oct 11, 2024
3430580
chore(coderd/notifications): half the number of db containers needed …
SasSwart Oct 11, 2024
9d47785
chore(coderd/notifications): ensure that all rendered golden files ar…
SasSwart Oct 11, 2024
7ab1664
chore(coderd/notifications): ensure that all rendered golden files ar…
SasSwart Oct 11, 2024
8f3ed16
chore(coderd/notifications): ensure that all rendered golden files ar…
SasSwart Oct 11, 2024
ca9b130
fix(coderd/notifications): use consistent labels throughout notificat…
SasSwart Oct 11, 2024
ff620e5
exempt notifications testdata from typo checking
SasSwart Oct 11, 2024
b08587c
chore(coderd/notifications) TestSMTP
SasSwart Oct 11, 2024
9927434
chore(coderd/notifications) rename mock_smtp package to match linting…
SasSwart Oct 11, 2024
f6ef5c4
chore(coderd/notifications) remove a defunct type conversion
SasSwart Oct 11, 2024
dadad9f
chore(coderd/notifications) rename mock_smtp package to match linting…
SasSwart Oct 11, 2024
f93135a
chore(coderd/notifications): fix golden file test
SasSwart Oct 11, 2024
55e3d02
chore(coderd/notifications): fix linting
SasSwart Oct 11, 2024
be00f59
chore(coderd/notifications): fix golden file test
SasSwart Oct 11, 2024
4991c33
chore(coderd/notifications): remove defunct test sleep
SasSwart Oct 14, 2024
464b244
chore(coderd/notifications): rename a test package
SasSwart Oct 14, 2024
3eb9c1e
chore(coderd/notifications): handle test assertions in the correct go…
SasSwart Oct 14, 2024
f049c05
chore(coderd/notifications): reformat imports and remove dead code
SasSwart Oct 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/typos.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,6 @@ extend-exclude = [
"tailnet/testdata/**",
"site/src/pages/SetupPage/countries.tsx",
"provisioner/terraform/testdata/**",
# notifications' golden files confuse the detector because of quoted-printable encoding
"coderd/notifications/testdata/**"
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
-- UserAccountCreated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' ||
E'New user account **{{.Labels.created_account_name}}** has been created.\n\n' ||
-- Mention the real name of the user who created the account:
E'This new user account was created for **{{.Labels.created_account_user_name}}** by **{{.Labels.account_creator}}**.'
WHERE
id = '4e19c0ac-94e1-4532-9515-d1801aa283b2';

-- UserAccountDeleted
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' ||
E'User account **{{.Labels.deleted_account_name}}** has been deleted.\n\n' ||
-- Mention the real name of the user who deleted the account:
E'The deleted account belonged to **{{.Labels.deleted_account_user_name}}** and was deleted by **{{.Labels.account_deleter_user_name}}**.'
WHERE
id = 'f44d9314-ad03-4bc8-95d0-5cad491da6b6';

-- UserAccountSuspended
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
E'User account **{{.Labels.suspended_account_name}}** has been suspended.\n\n' ||
-- Mention the real name of the user who suspended the account:
E'The newly suspended account belongs to **{{.Labels.suspended_account_user_name}}** and was suspended by **{{.Labels.account_suspender_user_name}}**.'
WHERE
id = 'b02ddd82-4733-4d02-a2d7-c36f3598997d';

-- YourAccountSuspended
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
E'Your account **{{.Labels.suspended_account_name}}** has been suspended by **{{.Labels.account_suspender_user_name}}**.'
WHERE
id = '6a2f0609-9b69-4d36-a989-9f5925b6cbff';


-- UserAccountActivated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
E'User account **{{.Labels.activated_account_name}}** has been activated.\n\n' ||
E'The newly activated account belongs to **{{.Labels.activated_account_user_name}}** and was activated by **{{.Labels.account_activator_user_name}}**.'
WHERE
id = '9f5af851-8408-4e73-a7a1-c6502ba46689';

-- YourAccountActivated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
E'Your account **{{.Labels.activated_account_name}}** has been activated by **{{.Labels.account_activator_user_name}}**.'
WHERE
id = '1a6a6bea-ee0a-43e2-9e7c-eabdb53730e4';
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
-- UserAccountCreated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' ||
E'New user account **{{.Labels.created_account_name}}** has been created.\n\n' ||
-- Use the conventional initiator label:
E'This new user account was created for **{{.Labels.created_account_user_name}}** by **{{.Labels.initiator}}**.'
WHERE
id = '4e19c0ac-94e1-4532-9515-d1801aa283b2';

-- UserAccountDeleted
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' ||
E'User account **{{.Labels.deleted_account_name}}** has been deleted.\n\n' ||
-- Use the conventional initiator label:
E'The deleted account belonged to **{{.Labels.deleted_account_user_name}}** and was deleted by **{{.Labels.initiator}}**.'
WHERE
id = 'f44d9314-ad03-4bc8-95d0-5cad491da6b6';

-- UserAccountSuspended
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
E'User account **{{.Labels.suspended_account_name}}** has been suspended.\n\n' ||
-- Use the conventional initiator label:
E'The newly suspended account belongs to **{{.Labels.suspended_account_user_name}}** and was suspended by **{{.Labels.initiator}}**.'
WHERE
id = 'b02ddd82-4733-4d02-a2d7-c36f3598997d';

-- YourAccountSuspended
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
-- Use the conventional initiator label:
E'Your account **{{.Labels.suspended_account_name}}** has been suspended by **{{.Labels.initiator}}**.'
WHERE
id = '6a2f0609-9b69-4d36-a989-9f5925b6cbff';

-- UserAccountActivated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
E'User account **{{.Labels.activated_account_name}}** has been activated.\n\n' ||
-- Use the conventional initiator label:
E'The newly activated account belongs to **{{.Labels.activated_account_user_name}}** and was activated by **{{.Labels.initiator}}**.'
WHERE
id = '9f5af851-8408-4e73-a7a1-c6502ba46689';

-- YourAccountActivated
UPDATE notification_templates
SET
body_template = E'Hi {{.UserName}},\n\n' || -- Add a \n
-- Use the conventional initiator label:
E'Your account **{{.Labels.activated_account_name}}** has been activated by **{{.Labels.initiator}}**.'
WHERE
id = '1a6a6bea-ee0a-43e2-9e7c-eabdb53730e4';
42 changes: 12 additions & 30 deletions coderd/notifications/dispatch/smtp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,8 @@ package dispatch_test

import (
"bytes"
"crypto/tls"
_ "embed"
"fmt"
"log"
"net"
"sync"
"testing"

Expand All @@ -22,6 +19,7 @@ import (
"github.com/coder/serpent"

"github.com/coder/coder/v2/coderd/notifications/dispatch"
"github.com/coder/coder/v2/coderd/notifications/dispatch/smtptest"
"github.com/coder/coder/v2/coderd/notifications/types"
"github.com/coder/coder/v2/codersdk"
"github.com/coder/coder/v2/testutil"
Expand All @@ -47,9 +45,9 @@ func TestSMTP(t *testing.T) {
subject = "This is the subject"
body = "This is the body"

caFile = "fixtures/ca.crt"
certFile = "fixtures/server.crt"
keyFile = "fixtures/server.key"
caFile = "smtptest/fixtures/ca.crt"
certFile = "smtptest/fixtures/server.crt"
keyFile = "smtptest/fixtures/server.key"
)

logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true, IgnoredErrorIs: []error{}}).Leveled(slog.LevelDebug)
Expand Down Expand Up @@ -125,7 +123,7 @@ func TestSMTP(t *testing.T) {

Auth: codersdk.NotificationsEmailAuthConfig{
Username: username,
PasswordFile: "fixtures/password.txt",
PasswordFile: "smtptest/fixtures/password.txt",
},
},
toAddrs: []string{to},
Expand Down Expand Up @@ -341,14 +339,14 @@ func TestSMTP(t *testing.T) {
cfg: codersdk.NotificationsEmailConfig{
TLS: codersdk.NotificationsEmailTLSConfig{
CAFile: caFile,
CertFile: "fixtures/nope.cert",
CertFile: "smtptest/fixtures/nope.cert",
KeyFile: keyFile,
},
},
// not using full error message here since it differs on *nix and Windows:
// *nix: no such file or directory
// Windows: The system cannot find the file specified.
expectedErr: "open fixtures/nope.cert:",
expectedErr: "open smtptest/fixtures/nope.cert:",
retryable: true,
},
{
Expand All @@ -358,13 +356,13 @@ func TestSMTP(t *testing.T) {
TLS: codersdk.NotificationsEmailTLSConfig{
CAFile: caFile,
CertFile: certFile,
KeyFile: "fixtures/nope.key",
KeyFile: "smtptest/fixtures/nope.key",
},
},
// not using full error message here since it differs on *nix and Windows:
// *nix: no such file or directory
// Windows: The system cannot find the file specified.
expectedErr: "open fixtures/nope.key:",
expectedErr: "open smtptest/fixtures/nope.key:",
retryable: true,
},
/**
Expand Down Expand Up @@ -417,7 +415,7 @@ func TestSMTP(t *testing.T) {

tc.cfg.ForceTLS = serpent.Bool(tc.useTLS)

backend := NewBackend(Config{
backend := smtptest.NewBackend(smtptest.Config{
AuthMechanisms: tc.authMechs,

AcceptedIdentity: tc.cfg.Auth.Identity.String(),
Expand All @@ -428,7 +426,7 @@ func TestSMTP(t *testing.T) {
})

// Create a mock SMTP server which conditionally listens for plain or TLS connections.
srv, listen, err := createMockSMTPServer(backend, tc.useTLS)
srv, listen, err := smtptest.CreateMockSMTPServer(backend, tc.useTLS)
require.NoError(t, err)
t.Cleanup(func() {
// We expect that the server has already been closed in the test
Expand Down Expand Up @@ -460,7 +458,7 @@ func TestSMTP(t *testing.T) {

// Wait for the server to become pingable.
require.Eventually(t, func() bool {
cl, err := pingClient(listen, tc.useTLS, tc.cfg.TLS.StartTLS.Value())
cl, err := smtptest.PingClient(listen, tc.useTLS, tc.cfg.TLS.StartTLS.Value())
if err != nil {
t.Logf("smtp not yet dialable: %s", err)
return false
Expand Down Expand Up @@ -522,19 +520,3 @@ func TestSMTP(t *testing.T) {
})
}
}

func pingClient(listen net.Listener, useTLS bool, startTLS bool) (*smtp.Client, error) {
tlsCfg := &tls.Config{
// nolint:gosec // It's a test.
InsecureSkipVerify: true,
}

switch {
case useTLS:
return smtp.DialTLS(listen.Addr().String(), tlsCfg)
case startTLS:
return smtp.DialStartTLS(listen.Addr().String(), tlsCfg)
default:
return smtp.Dial(listen.Addr().String())
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package dispatch_test
package smtptest

import (
"crypto/tls"
Expand Down Expand Up @@ -162,7 +162,7 @@ func (*Session) Reset() {}
func (*Session) Logout() error { return nil }

// nolint:revive // Yes, useTLS is a control flag.
func createMockSMTPServer(be *Backend, useTLS bool) (*smtp.Server, net.Listener, error) {
func CreateMockSMTPServer(be *Backend, useTLS bool) (*smtp.Server, net.Listener, error) {
// nolint:gosec
tlsCfg := &tls.Config{
GetCertificate: readCert,
Expand Down Expand Up @@ -203,3 +203,19 @@ func readCert(_ *tls.ClientHelloInfo) (*tls.Certificate, error) {

return &crt, nil
}

func PingClient(listen net.Listener, useTLS bool, startTLS bool) (*smtp.Client, error) {
tlsCfg := &tls.Config{
// nolint:gosec // It's a test.
InsecureSkipVerify: true,
}

switch {
case useTLS:
return smtp.DialTLS(listen.Addr().String(), tlsCfg)
case startTLS:
return smtp.DialStartTLS(listen.Addr().String(), tlsCfg)
default:
return smtp.Dial(listen.Addr().String())
}
}
Loading
Loading