NotificationsYou must be signed in to change notification settings
Fork927
Star10.1k

chore: add agentapi tests#11269

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

deansheather merged 7 commits intomainfromdean/agent-api-tests

Jan 26, 2024

Merged

chore: add agentapi tests#11269

deansheather merged 7 commits intomainfromdean/agent-api-tests

Jan 26, 2024

Conversation

Copy link

Member

deansheather commentedDec 18, 2023•
edited
Loading

The tests added in this PR can be reviewed now as I probably won't be changing them (other than review comments). I'll be adding more tests for the files TODOd below in upcoming commits and I'll re-request review.

In most cases the new tests have more cases/coverage than the old ones which is nice. Some of them are quite verbose though, as I tried to maximize usage of fields on types and enums (to get conversion coverage).

TODOs:

metadata.go
stats.go
tailnet.go

chore: add agentapi tests

36bfcbd

deansheather requested a review fromspikecurtis

December 18, 2023 18:16

deansheather self-assigned this

Dec 18, 2023

deansheather marked this pull request as draft

December 18, 2023 18:16

Remaining tests

24f6614

deansheather marked this pull request as ready for review

December 19, 2023 12:12

Copy link

MemberAuthor

deansheather commentedDec 19, 2023•
edited
Loading

main
- chore: add agentapi tests #11269 👈
  - chore: convert workspaceagent HTTP API to agentapi #11280

This stack of pull requests is managed by Graphite.Learn more about stacking.

Join@deansheather and the rest of your teammates onGraphite

deansheather mentioned this pull request

Dec 19, 2023

chore: convert workspaceagent HTTP API to agentapi#11280

Closed

avoid race in test

3b9d365

spikecurtis reviewed

Dec 19, 2023

View reviewed changes

Copy link

Contributor

spikecurtis left a comment

Choose a reason for hiding this comment

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

I didn't finish this review, but I'm sending the comments I have since you're in a different timezone

coderd/agentapi/apps_test.go OutdatedShow resolvedHide resolved

coderd/agentapi/lifecycle_test.go OutdatedShow resolvedHide resolved

spikecurtis reviewed

Dec 20, 2023

View reviewed changes

coderd/agentapi/logs_test.go OutdatedShow resolvedHide resolved

coderd/agentapi/manifest_test.go

		},
		}
		}
		)

Copy link

Contributor

spikecurtisDec 20, 2023

Choose a reason for hiding this comment

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

I wonder if we should break the manifest up into separate API calls. It's kind of a dumping ground for lots of different, unrelated kinds of information...

Not that I'm expecting it in this PR.

coderd/agentapi/metadata_test.go OutdatedShow resolvedHide resolved

coderd/agentapi/stats_test.go OutdatedShow resolvedHide resolved

coderd/agentapi/stats_test.go

		now, err := time.Parse("2006-01-02 15:04:05 -0700 MST", "2023-12-19 07:30:00 +1100 AEDT")
		require.NoError(t, err)
		now = dbtime.Time(now)
		nextAutostart := now.Add(30 * time.Minute).UTC() // always sent to DB as UTC

Copy link

Contributor

spikecurtisDec 20, 2023

Choose a reason for hiding this comment

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

to me, this smells like we need to refactor. The AgentAPI should not be in the business of computing autostart times. There should be a scheduler component that gets an activity indication from the Agent API and it computes and decides how to update the autostart / autostop schedule.

Another one for some other PR...