Is there a reason we don't want to surface an error? It seems like an inconsistency to me that a workspace is being bumped but doesn't exist.

Since the bump occurs asynchronously, there is no guarantee the workspace exists when this line executes. But, that is not an error.

That makes sense 👍🏻. Should we btw consider moving the log above to blow the error handling?

That was an artifact from testing so I will remove.

This might not trigger a CI flake currently, but its behavior is racy. We can't guarantee that the goroutine has run to completion here so we also can't guarantee we're testing the correct thing.

Yeah, it sucks, but I couldn't think of a clean alternative.

One option would be dependency injection, e.g. store the activity bumping method on coderd.API, replace it with a wrapper in the test that can inform e.g. on a channel when it has run.

This would be a bit cleaner as an internal test so that nothing needs to be exported.

Another idea: I'm not sure what we're sending on the websocket, but could we write a payload informing the other side about what we did (e.g. event: bumped workspace)?

One option would be dependency injection, e.g. store the activity bumping method on coderd.API, replace it with a wrapper in the test that can inform e.g. on a channel when it has run.

In this particular test we're looking for an absence of a call. So, it's not as simple as replacing the activity bump method. We should also strive to test behavior from the user's perspective. The more we dependency inject and replace internals in our tests the further away we're getting from the user. We may be better off accepting a little bit of raciness for more test accuracy here.

This would be a bit cleaner as an internal test so that nothing needs to be exported.

I, too, enjoy internal tests, but that is a codebase decision larger than this PR.

I added an explanatory comment to the code. Let me know your thoughts.

In this particular test we're looking for an absence of a call.

I thought we would be testing for the presence of a call, but the absence of modification? What’s the purpose of testing for the absence of a call?

The only case that the call wouldn’t happen is if the updateDB variable is false which seems like a detail that shouldn’t be tested in this test. If we really need to test that case then we should try to expose that some way so we don’t have racy tests.

In my personal opinion we should avoid test racyness as much as possible because those lead to CI flakes that in turn result in loss of developer productivity.

In the current implementation, since updateDB is false, there be no call. Let's say we restructure the code so it always calls a replaceable function. In that case, there would still be raciness because we're looking to assert that the bump never happens. We can't wait for infinity, so we'd have to wait for some period or some event. We could wait for X ticks of the Agent Stat Loop, but there's still raciness.