chore: Add test helpers to improve coverage #166

kylecarbs · 2022-02-05T08:02:35Z

This removes the Server struct from coderdtest. I don't believe we should ever need anything but an API client to test all functionality against coderd. If we do, there's probably a bad abstraction in play.

I've adopted a naming scheme of Test<HTTP handler> for each route. I'll be creating a custom linter for it in rules.go.

Version more accurately represents version storage. This forks from the WorkspaceHistory name, but I think it's easier to understand Workspace history.

codecov · 2022-02-05T08:09:03Z

Codecov Report

Merging #166 (a8a0531) into main (f19770b) will increase coverage by 2.01%.
The diff coverage is 74.77%.

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
+ Coverage   65.44%   67.45%   +2.01%     
==========================================
  Files         106      106              
  Lines        5580     5636      +56     
  Branches       68       68              
==========================================
+ Hits         3652     3802     +150     
+ Misses       1572     1475      -97     
- Partials      356      359       +3

Flag	Coverage Δ
unittest-go-macos-latest	`65.24% <66.97%> (+2.50%)`	⬆️
unittest-go-ubuntu-latest	`66.55% <72.47%> (+2.20%)`	⬆️
unittest-go-windows-latest	`64.94% <66.97%> (+2.62%)`	⬆️
unittest-js	`64.21% <ø> (ø)`

Impacted Files	Coverage Δ
coderd/coderd.go	`93.42% <ø> (-0.09%)`	⬇️
coderd/provisioners.go	`84.21% <0.00%> (+13.15%)`	⬆️
coderd/users.go	`59.32% <0.00%> (-2.32%)`	⬇️
codersdk/client.go	`51.51% <33.33%> (-4.23%)`	⬇️
provisioner/echo/serve.go	`47.36% <38.88%> (-5.08%)`	⬇️
provisionerd/provisionerd.go	`68.64% <40.00%> (+0.69%)`	⬆️
coderd/projectversion.go	`66.07% <60.00%> (+4.92%)`	⬆️
coderd/workspacehistorylogs.go	`58.86% <60.00%> (+6.41%)`	⬆️
coderd/workspaces.go	`65.11% <72.72%> (-1.24%)`	⬇️
coderd/workspacehistory.go	`61.78% <75.00%> (+8.05%)`	⬆️
... and 23 more

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 f19770b...a8a0531. Read the comment docs.

bryphe-coder · 2022-02-05T20:39:20Z

coderd/projects_test.go

-		})
+		client := coderdtest.New(t)
+		_ = coderdtest.CreateInitialUser(t, client)
+		projects, err := client.Projects(context.Background(), "")


I really like how much simpler this is 🎉

bryphe-coder · 2022-02-05T20:42:41Z

coderd/workspacehistorylogs.go

+		if logs == nil {
+			logs = []database.WorkspaceHistoryLog{}
+		}


I wonder if this is the same issue I saw with projects? Sometimes, I'd get nil for projects returned - even in the case where was no error (and had to do something similar): https://github.com/coder/coder/pull/140/files#r797267887

I didn't get a chance to dive deeper yet (I'll look at it next week) - but wondering if its the same issue here? And maybe a bug with our query or sqlc integration?

Yup, it is! Not an issue with our sqlc implementation, just a false assumption on my part! We now test for this!

Cool, good to know! I didn't realize that there could be a case where those functions could return nil (I thought they'd always return an empty array) - but something I'll watch out for now.

coderd/workspaces.go

bryphe-coder · 2022-02-05T20:45:03Z

rules.go

+// Use xerrors everywhere! It provides additional stacktrace info!
+//nolint:unused,deadcode,varnamelen
+func xerrors(m dsl.Matcher) {


Thanks for adding this 👍

bryphe-coder · 2022-02-05T20:45:45Z

provisionerd/provisionerd.go

@@ -220,7 +224,7 @@ func (p *provisionerDaemon) runJob(ctx context.Context, job *proto.AcquiredJob)
 				JobId: job.JobId,
 			})
 			if err != nil {
-				go p.cancelActiveJobf("send periodic update: %s", err)
+				p.cancelActiveJobf("send periodic update: %s", err)


Was this intentional to remove all these? It might make sense to split this out to a separate change, if possible - just in case there is flakiness or a race condition introduced, it'll be easier to track down.

It was intentional! The job was hitting a finished state even though it was supposed to cancel itself.

We can separate it out if you feel strongly. It was a logical lapse when creating provisionerd for sure.

Co-authored-by: Bryan <bryan@coder.com>

bryphe-coder · 2022-02-05T20:48:50Z

coderd/provisionerdaemons.go

-		provisionerState := []byte{}
-		// If workspace history exists before this entry, use that state.
-		// We can't use the before state everytime, because if a job fails
-		// for some random reason, the workspace shouldn't be reset.
-		//
-		// Maybe we should make state global on a workspace?
-		if workspaceHistory.BeforeID.Valid {
-			beforeHistory, err := server.Database.GetWorkspaceHistoryByID(ctx, workspaceHistory.BeforeID.UUID)
-			if err != nil {
-				return nil, failJob(fmt.Sprintf("get workspace history: %s", err))
-			}
-			provisionerState = beforeHistory.ProvisionerState
-		}


Is this not needed, or is it handled elsewhere? Wasn't clear to me why this could be removed

Not needed. This wasn't tested before, and wasn't actually necessary.

bryphe-coder

Just some minor nits, but overall looks like a really nice improvement to our tests 🎉 Thanks for doing this cleanup @kylecarbs !

jawnsy · 2022-02-05T20:31:01Z

.golangci.yml

+    settings:
+      ruleguard:
+        failOn: all
+        rules: rules.go


suggest using separate files for ruleguard like we do in the monorepo (and using a wildcard here), it helps keep each rule organized, and gives us a way to group related rules

I'd prefer for us to wait until this becomes more unruly.

rules.go

jawnsy · 2022-02-05T20:34:01Z

provisionerd/provisionerd.go

 		return
 	}

 	err := os.MkdirAll(p.opts.WorkDirectory, 0700)
 	if err != nil {
-		go p.cancelActiveJobf("create work directory %q: %s", p.opts.WorkDirectory, err)
+		p.cancelActiveJobf("create work directory %q: %s", p.opts.WorkDirectory, err)


nice, doing this sync seems like it'll be easier to follow

jawnsy · 2022-02-05T20:37:06Z

provisionerd/provisionerd.go

@@ -54,7 +55,8 @@ func New(clientDialer Dialer, opts *Options) io.Closer {
 		closeCancel: ctxCancel,
 		closed:      make(chan struct{}),

-		jobRunning: make(chan struct{}),
+		jobRunning:   make(chan struct{}),
+		jobCancelled: *atomic.NewBool(true),


how come we need to use this atomic class if we're already using locks to protect things?

Job cancel and completion need to be grouped separately. I tried to make them one before this, but it became more difficult to manage.

If two cancels come immediately after one another, it's not good to lock on a mutex and wait for the other to complete, because a job may cancel itself at the same time provisionerd is canceling it, and lock forever.

jawnsy · 2022-02-05T20:37:52Z

provisioner/echo/serve_test.go

@@ -53,7 +53,7 @@ func TestEcho(t *testing.T) {
 				},
 			},
 		}}
-		data, err := echo.Tar(responses, nil)
+		data, err := echo.Tar(&echo.Responses{responses, nil})


personally I find this way of defining structs to be less readable, and prefer the variant that lists fields. this is also order-dependent.

I agree. I'll adjust this!

jawnsy · 2022-02-05T20:41:39Z

database/dump.sql

+    id uuid NOT NULL,
+    created_at timestamp with time zone NOT NULL,
+    project_version_id uuid NOT NULL,
+    name character varying(64) NOT NULL,


just curious, are there benefits to defining columns as varchar vs text? or is it just cosmetic?

Just a nice restriction to prevent someone from making a 10MB name accidentally.

jawnsy · 2022-02-05T20:44:02Z

codersdk/users_test.go


-	t.Run("CreateMultiple", func(t *testing.T) {
+func TestUserOrganizations(t *testing.T) {


do these functions handle context cancellation? might be nice to add tests to check that the server won't do anything if the context has been cancelled and/or that it can abandon in-progress requests and return early in that case

The t.Cleanup on coderdtest.New will close all connections, so shouldn't be possible.

I'm adding goleak now though so human error can't exist here. Good suggestion!

jawnsy · 2022-02-05T20:45:35Z

codersdk/client.go

@@ -112,5 +113,9 @@ func (e *Error) StatusCode() int {
 }

 func (e *Error) Error() string {
-	return fmt.Sprintf("status code %d: %s", e.statusCode, e.Message)
+	msg := fmt.Sprintf("status code %d: %s", e.statusCode, e.Message)


it doesn't really matter unless this is a hot path, but using strings.Builder and Fprintf to construct these will reduce allocations

Good suggestion regardless! Updated it.

jawnsy · 2022-02-05T20:47:41Z

coderd/workspacehistorylogs.go

 		}
 		if err != nil {
 			httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{
 				Message: fmt.Sprintf("get workspace history: %s", err),
 			})
 			return
 		}
+		if logs == nil {
+			logs = []database.WorkspaceHistoryLog{}


I guess this is just to get the JSON to return an empty array instead of nil?

jawnsy · 2022-02-05T20:52:27Z

coderd/projectversion.go

-			return
-		}
-	default:
+	tarReader := tar.NewReader(bytes.NewReader(createProjectVersion.StorageSource))


should there be a content negotiation check here? i.e. client should use a Content-Type: application/x-tar on the upload, and server can return 415 Unsupported Media Type - that would allow us to accept other content types in the future

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/415

I suppose the answer is maybe. Right now, the archive is sent as base64 encoded bytes in JSON. That's not ideal, and we'll probably swap it to a file-upload format like mentioned.

kylecarbs added 5 commits February 5, 2022 01:01

chore: Rename ProjectHistory to ProjectVersion

7f2a99f

Version more accurately represents version storage. This forks from the WorkspaceHistory name, but I think it's easier to understand Workspace history.

Rename files

7c46342

Standardize tests a bit more

9e04a0e

Remove Server struct from coderdtest

9cd8b43

Improve test coverage for workspace history

be3e6ac

kylecarbs self-assigned this Feb 5, 2022

Merge branch 'main' into testcleanup

6e4ca5a

Fix linting errors

ab4dcb1

kylecarbs force-pushed the testcleanup branch 2 times, most recently from a703482 to b2d6337 Compare February 5, 2022 08:36

Fix coderd test leak

57efae2

kylecarbs force-pushed the testcleanup branch from b2d6337 to 57efae2 Compare February 5, 2022 08:37

Fix coderd test leak

9fa815c

kylecarbs force-pushed the testcleanup branch 4 times, most recently from 13ef836 to 16449d8 Compare February 5, 2022 17:48

Merge branch 'testcleanup' of github.com:coder/coder into testcleanup

0fb2a5a

kylecarbs force-pushed the testcleanup branch from 16449d8 to 0fb2a5a Compare February 5, 2022 18:01

kylecarbs added 2 commits February 5, 2022 18:39

Improve workspace history logs

c50cee0

Standardize test structure for codersdk

e202b2d

kylecarbs marked this pull request as ready for review February 5, 2022 20:11

kylecarbs requested a review from bryphe-coder February 5, 2022 20:11

kylecarbs added 2 commits February 5, 2022 20:17

Fix linting errors

ca31d90

Fix WebSocket compression

673c59d

bryphe-coder reviewed Feb 5, 2022

View reviewed changes

coderd/workspaces.go Outdated Show resolved Hide resolved

bryphe-coder reviewed Feb 5, 2022

View reviewed changes

Update coderd/workspaces.go

aad577f

Co-authored-by: Bryan <bryan@coder.com>

bryphe-coder reviewed Feb 5, 2022

View reviewed changes

bryphe-coder approved these changes Feb 5, 2022

View reviewed changes

jawnsy reviewed Feb 5, 2022

View reviewed changes

Add test for listing project parameters

6c39920

kylecarbs force-pushed the testcleanup branch 3 times, most recently from b900f18 to 479397d Compare February 5, 2022 22:57

Cache npm dependencies with setup node

a4d17d0

kylecarbs force-pushed the testcleanup branch 2 times, most recently from 95c711e to b900f18 Compare February 5, 2022 23:16

Remove windows npm cache key

a8a0531

kylecarbs force-pushed the testcleanup branch from ad31063 to a8a0531 Compare February 5, 2022 23:51

kylecarbs merged commit 1796dc6 into main Feb 6, 2022

kylecarbs deleted the testcleanup branch February 6, 2022 00:24

bryphe-coder mentioned this pull request Feb 9, 2022

fix: /projects endpoint returning null instead of empty array #140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Add test helpers to improve coverage #166

chore: Add test helpers to improve coverage #166

kylecarbs commented Feb 5, 2022 •

edited

Loading

codecov bot commented Feb 5, 2022 •

edited

Loading

bryphe-coder Feb 5, 2022

bryphe-coder Feb 5, 2022

kylecarbs Feb 5, 2022

bryphe-coder Feb 5, 2022

bryphe-coder Feb 5, 2022

bryphe-coder Feb 5, 2022

kylecarbs Feb 5, 2022

bryphe-coder Feb 5, 2022

kylecarbs Feb 5, 2022

bryphe-coder left a comment

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022

jawnsy Feb 5, 2022

kylecarbs Feb 5, 2022


		t.Run("CreateMultiple", func(t *testing.T) {
		func TestUserOrganizations(t *testing.T) {

chore: Add test helpers to improve coverage #166

chore: Add test helpers to improve coverage #166

Conversation

kylecarbs commented Feb 5, 2022 • edited Loading

codecov bot commented Feb 5, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryphe-coder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylecarbs commented Feb 5, 2022 •

edited

Loading

codecov bot commented Feb 5, 2022 •

edited

Loading