Skip to content

chore: rename FakeCoordinator for export #11991

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 5, 2024
Merged

chore: rename FakeCoordinator for export #11991

merged 1 commit into from
Feb 5, 2024

Conversation

spikecurtis
Copy link
Contributor

@spikecurtis spikecurtis commented Feb 2, 2024 •
edited
Loading

Part of a stack that fixes graceful disconnect from the CLI to tailnet. I reuse FakeCoordinator in a test for graceful disconnects.

@spikecurtis Graphite App
Copy link
Contributor Author

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

Join @spikecurtis and the rest of your teammates on Graphite Graphite

This was referenced Feb 2, 2024
Merged
Merged
@spikecurtis spikecurtis marked this pull request as ready for review February 2, 2024 09:17
johnstcn
johnstcn reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Leaving the exported methods ServeAgent, Close, ServeMultiAgent etc. as just panicking is likely to cause an unwelcome surprise to a later unwary user.

I'd feel better if we passed in a *testing.T into FakeCoordinator and call t.Fail() if these methods are called, as that will at least give some more useful indication of where to look. Panicking right now in tests does not produce a useful stacktrace.

@spikecurtis Graphite App
Copy link
Contributor Author

Here's what I get when we hit a panic:

=== RUN   TestConfigMaps_setBlockEndpoints_different
=== PAUSE TestConfigMaps_setBlockEndpoints_different
=== CONT  TestConfigMaps_setBlockEndpoints_different
panic: do I get a stacktrace?

goroutine 118 [running]:
github.com/coder/coder/v2/tailnet/tailnettest.(*FakeCoordinator).ServeClient(0x1400062c2b0?, {0x1?, 0x1?}, {0x10, 0x0, 0x0, 0x1, 0x0, 0x0, 0x0, ...}, ...)
	/Users/spike/repos/coder/tailnet/tailnettest/tailnettest.go:267 +0x2c
github.com/coder/coder/v2/tailnet.(*ClientService).ServeClient(0x140007a2070, {0x101c86320, 0x14000616420}, {0x101791764, 0x3}, {0x101c8d440, 0x14000140300}, {0x10, 0x0, 0x0, ...}, ...)
	/Users/spike/repos/coder/tailnet/service.go:92 +0x3c4
github.com/coder/coder/v2/tailnet_test.TestClientService_ServeClient_V1.func1()
	/Users/spike/repos/coder/tailnet/service_test.go:122 +0x60
created by github.com/coder/coder/v2/tailnet_test.TestClientService_ServeClient_V1
	/Users/spike/repos/coder/tailnet/service_test.go:121 +0x444

vs t.Fatal

=== RUN   TestClientService_ServeClient_V1
=== PAUSE TestClientService_ServeClient_V1
=== CONT  TestClientService_ServeClient_V1
    tailnettest.go:268: do I get a stacktrace?
    service_test.go:127: timeout
--- FAIL: TestClientService_ServeClient_V1 (10.00s)

vs t.Error --- since we "shouldn't" use Fatal as these are basically always called from a goroutine.

=== RUN   TestClientService_ServeClient_V1
=== PAUSE TestClientService_ServeClient_V1
=== CONT  TestClientService_ServeClient_V1
    tailnettest.go:268: do I get a stacktrace?
    service_test.go:123: ServeClient returned; err=test error
--- FAIL: TestClientService_ServeClient_V1 (0.00s)

so, anyway, the only way to get a stacktrace is panic.

@spikecurtis spikecurtis force-pushed the spike/fake-coordinator-rename branch from 149acdb to dbd0a93 Compare February 5, 2024 06:45
@spikecurtis spikecurtis requested a review from johnstcn February 5, 2024 06:46
@spikecurtis Graphite App
Copy link
Contributor Author

spikecurtis commented Feb 5, 2024 •
edited
Loading

Merge activity

@spikecurtis spikecurtis merged commit 646ac94 into main Feb 5, 2024
@spikecurtis spikecurtis deleted the spike/fake-coordinator-rename branch February 5, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnstcn johnstcn johnstcn approved these changes

@coadler coadler Awaiting requested review from coadler

Assignees

@spikecurtis spikecurtis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@spikecurtis @johnstcn