Add timeout for /health endpoint checks

[cristinaleonr]

This change is 

[stephen-soltesz]

Reviewed 1 of 6 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)

---

cmd/heartbeat/health/checker_test.go line 28 at r1 (raw file):

				},
				&EndpointClient{
					Client: http.Client{},

nit: the Client value is a literal (not a pointer) and the zero value for http.Client is what's being declared here. Is it sufficient to simply declare &EndpointClient{} here?

---

cmd/heartbeat/health/checker_test.go line 151 at r1 (raw file):

				},
				&EndpointClient{
					Client: http.Client{},

Do any of these cases induce a timeout event for the health endpoint client? Which one if so? Please add one if not. This is the functionality being added. One test verifying that behavior would be welcome.

[cristinaleonr]

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

---

cmd/heartbeat/health/checker_test.go line 28 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

nit: the Client value is a literal (not a pointer) and the zero value for http.Client is what's being declared here. Is it sufficient to simply declare &EndpointClient{} here?

Changed.

---

cmd/heartbeat/health/checker_test.go line 151 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Do any of these cases induce a timeout event for the health endpoint client? Which one if so? Please add one if not. This is the functionality being added. One test verifying that behavior would be welcome.

I have added a test in the test file for the class where the behavior was implemented.

[stephen-soltesz]

Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)

---

cmd/heartbeat/health/endpoint-check_test.go line 55 at r2 (raw file):

			}

			hc := NewEndpointClient(time.Second)

Where is tt.timeout used?

[cristinaleonr]

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

---

cmd/heartbeat/health/endpoint-check_test.go line 55 at r2 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Where is tt.timeout used?

Whoops, sorry! The test was passing for a different reason, which I have now fixed.

[cristinaleonr]

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

---

cmd/heartbeat/health/endpoint-check_test.go line 55 at r2 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Whoops, sorry! The test was passing for a different reason, which I have now fixed.

Nvm, wait.

[stephen-soltesz]

Reviewable status: 0 of 1 approvals obtained (waiting on @cristinaleonr)

---

cmd/heartbeat/health/endpoint-check_test.go line 31 at r3 (raw file):

			name:     "timeout",
			code:     http.StatusOK,
			timeout:  0,

This error case is labeled "timeout" -- but iiuc, an http.Client behavior with timeout = 0 is no timeout. What is this case testing?

---

cmd/heartbeat/health/endpoint-check_test.go line 45 at r3 (raw file):

		},
		{
			name:    "error",

What is the error case? Please use a slightly more descriptive name.

---

cmd/heartbeat/health/endpoint-check_test.go line 53 at r3 (raw file):

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			if !tt.wantErr {

Where is tt.startSrv used?

[cristinaleonr]

Apologies for the multiple revisions! I double checked it now 😅.

Reviewable status: 0 of 1 approvals obtained (waiting on @stephen-soltesz)

---

cmd/heartbeat/health/endpoint-check_test.go line 31 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

This error case is labeled "timeout" -- but iiuc, an http.Client behavior with timeout = 0 is no timeout. What is this case testing?

I separated this into a new test with a 1 second timeout.

---

cmd/heartbeat/health/endpoint-check_test.go line 45 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

What is the error case? Please use a slightly more descriptive name.

The server is not running. Done.

---

cmd/heartbeat/health/endpoint-check_test.go line 53 at r3 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Where is tt.startSrv used?

Sorry, I removed it and just moved the test case to its own function.

I realized that and that's why I added the "Nvm, wait" comment 👼.

[stephen-soltesz]

One suggested refinement if possible.

Reviewed 3 of 6 files at r1, 1 of 2 files at r2, 1 of 2 files at r4.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @cristinaleonr)

---

cmd/heartbeat/health/endpoint-check_test.go line 66 at r4 (raw file):

	got, err := hc.checkHealthEndpoint()
	if err == nil {
		t.Errorf("checkHealthEndpoint() error = %v, wantErr %s", err, "context deadline error")

Is it possible to add an affirmative errors.Is check for context deadline error? I see context.DeadlineExceeded -- this would be nice if possible. Not blocking if not.

---

cmd/heartbeat/health/healthtest/healthtest.go line 18 at r4 (raw file):

}

func TestTimeoutServer(timeout time.Duration) *httptest.Server {

👍

[cristinaleonr]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)

---

cmd/heartbeat/health/endpoint-check_test.go line 66 at r4 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Is it possible to add an affirmative errors.Is check for context deadline error? I see context.DeadlineExceeded -- this would be nice if possible. Not blocking if not.

Even though the error message is of the form Get "http://127.0.0.1:44235/health": context deadline exceeded (Client.Timeout exceeded while awaiting headers), it must not specifically wrap that error since the errors.Is check returns false.

[cristinaleonr]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)

---

cmd/heartbeat/health/endpoint-check_test.go line 66 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Even though the error message is of the form Get "http://127.0.0.1:44235/health": context deadline exceeded (Client.Timeout exceeded while awaiting headers), it must not specifically wrap that error since the errors.Is check returns false.

Agh, hold on again, please.

[cristinaleonr]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)

---

cmd/heartbeat/health/endpoint-check_test.go line 66 at r4 (raw file):

Previously, cristinaleonr (Cristina Leon) wrote…

Agh, hold on again, please.

Yep, my original statement above is right (I was double checking 👼).

[cristinaleonr]

Related to #149.