Add integ test for http server

[bhuber]

Overview

This fixes #40 , hopefully satisfactorily. The intent here is to make a generic and flexible test harness for exercising the gofail http api. I've added some obvious basic tests; I'm sure the maintainers can think of more to add in the future. I also fixed several inaccuracies in the documentation, and added a few missing pieces I found during the course of implementation.

The code should be pretty self explanatory. The main explanation of the test setup is in the godoc for TestAll in integration/server/server_test.go .

This is my first PR to this repo, and I did my best to follow all the contribution instructions I could find, but please let me know if I missed anything.

Testing

• make run-all-integration-tests works locally
• I ran the github actions in my fork, here's the passing integration test run: https://github.com/bhuber/gofail/actions/runs/13305753815

[ahrtr]

cc @henrybear327, @ivanvc and @ArkaSaha30 for comments

[ivanvc]

Thanks for the pull request, @bhuber, and for asking me to review this. I'm not very familiar with gofail's codebase. However, I read and executed the code locally to check the integration test. I left a couple of comments. Thanks again, Bennet.

[ivanvc]

Are these functions intentionally named with the Example prefix so they appear in the godoc? If not, renaming them may be a good idea to avoid confusion.

[bhuber]

I copied these from https://github.com/etcd-io/gofail/blob/master/examples/examples.go , so felt it best to remain consistent as much as possible. It's definitely worth adding a comment to explain that. I'll do that before merging, but hold off for now in case there's more to change.

[ivanvc]

Should we still keep the documentation for a single endpoint? Maybe push this example before activating the multiple endpoints?

[bhuber]

This isn't removing the documentation for a single endpoint, it's fixing it. The previous URL was nonsensical (why would you try to assign a return("hello") value in the same operation that's listing current values?) and returns an error at runtime

[bhuber]

After re-examining the readme, I've just realized the "get all failpoint values" API isn't documented here ("listall" in server_test.go", so that's definitely worth adding. Maybe that's what you were referring to?

[ivanvc]

This isn't removing the documentation for a single endpoint, it's fixing it. The previous URL was nonsensical (why would you try to assign a return("hello") value in the same operation that's listing current values?) and returns an error at runtime

I got it, it makes sense. Thanks for the clarification.

After re-examining the readme, I've just realized the "get all failpoint values" API isn't documented here ("listall" in server_test.go", so that's definitely worth adding. Maybe that's what you were referring to?

Unfortunately, as I said, I'm not an expert on this code base. So, I read the documentation as an end user and was confused. Thanks for the clarification. I would suggest to do this in a follow-up PR :)

[bhuber]

Thanks for the pull request, @bhuber, and for asking me to review this. I'm not very familiar with gofail's codebase. However, I read and executed the code locally to check the integration test. I left a couple of comments. Thanks again, Bennet.

Thanks for taking the time to read it, I know it's a bit lengthy! Replied to your comments, hoping for more feedback before making changes, as they're pretty minor.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bhuber
Once this PR has been reviewed and has the lgtm label, please ask for approval from ahrtr. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[bhuber]

I made the changes requested, but squashed into the previous commit, so they unfortunately can't be easily compared. I'm not entirely sure what the best workflow here is. On the one hand, it looks like this project really prefers one signed-off commit per PR, so merges need to be squashed and signed somehow. On the other, this makes comparing revisions within a PR very difficult for reviewers. I'm open to any suggestions for how to handle this in the future.

The changes are quite small, for your convenience here's the entirety of them since the last version: https://gist.github.com/bhuber/cbf4a366fb63a6689bfb15619d637132

[ivanvc]

Squashing is fine :) Thanks for addressing my comments, @bhuber!

I'd like to invoke @henrybear327, as he's more familiar with the code base and the integration tests. However, I know he's busy with some work related to the v3.6.0 release in the etcd repository. So, it may take some time until he can take a look at your pull request.

[ivanvc]

/ok-to-test

Edit: This repository's /ok-to-test workflow is missing. We need a maintainer to approve the workflows manually.

[bhuber]

Gentle bump - it's been over a week since last update

[henrybear327]

We might want to bump this to align with the latest go version update!