chore: durability tests #80
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
WalkthroughThis pull request introduces a new durability test for the Beanstalk server, along with associated helper functions and configurations. It also modifies the CI/CD pipeline to accommodate these changes. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files ignored due to filter (3)
- go.work.sum
- tests/go.mod
- tests/go.sum
Files selected for processing (5)
- .github/workflows/linux.yml (1 hunks)
- .github/workflows/linux_durability.yml (1 hunks)
- tests/configs/.rr-beanstalk-durability-redial.yaml (1 hunks)
- tests/helpers/helpers.go (2 hunks)
- tests/jobs_beanstalk_durability_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/linux.yml
- tests/configs/.rr-beanstalk-durability-redial.yaml
Additional comments (Suppressed): 5
.github/workflows/linux_durability.yml (4)
1-99: The new GitHub Actions workflow
linux_durability.ymlseems well-structured and follows best practices for setting up the environment, caching dependencies, running tests, and uploading coverage results. However, there are a few points that need to be verified or refined.16-22: Ensure that the matrix configuration is correct and matches your testing requirements. The current configuration only tests on Ubuntu with PHP 8.2 and the stable version of Go. If you need to support other versions or operating systems, you should add them to the matrix.
70-71: The test command runs a specific Go file (
jobs_beanstalk_durability_test.go). If there are other test files that need to be included in this workflow, you should modify the command accordingly. Also, ensure that the-timeoutvalue is appropriate for your tests.- go test -timeout 20m -v -race -cover -tags=debug -failfast -coverpkg=$(cat pkgs.txt) -coverprofile=./coverage-ci/beanstalk_dur.out -covermode=atomic jobs_beanstalk_durability_test.go + go test -timeout 20m -v -race -cover -tags=debug -failfast -coverpkg=$(cat pkgs.txt) -coverprofile=./coverage-ci/beanstalk_dur.out -covermode=atomic ./...
- 93-94: This command filters out lines not containing 'roadrunner' from the second line onwards in the coverage summary. Ensure that this filtering is necessary and correctly implemented for your use case.
tests/helpers/helpers.go (1)
- 1-9: The new hunk introduces additional imports (
bytes,net/http) to thehelperspackage. These are used in the newly added functions for managing a Toxiproxy proxy.
| func TestDurabilityBeanstalk(t *testing.T) { | ||
| newClient := toxiproxy.NewClient("127.0.0.1:8474") | ||
|
|
||
| _, err := newClient.CreateProxy("redial", "127.0.0.1:11400", "127.0.0.1:11300") | ||
| require.NoError(t, err) | ||
| defer helpers.DeleteProxy("redial", t) | ||
|
|
||
| cont := endure.New(slog.LevelDebug, endure.GracefulShutdownTimeout(time.Second*60)) | ||
|
|
||
| cfg := &config.Plugin{ | ||
| Version: "2.9.0", | ||
| Path: "configs/.rr-beanstalk-durability-redial.yaml", | ||
| Prefix: "rr", | ||
| } | ||
|
|
||
| err = cont.RegisterAll( | ||
| cfg, | ||
| &server.Plugin{}, | ||
| &rpcPlugin.Plugin{}, | ||
| &logger.Plugin{}, | ||
| &jobs.Plugin{}, | ||
| &resetter.Plugin{}, | ||
| &informer.Plugin{}, | ||
| &beanstalk.Plugin{}, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| err = cont.Init() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| ch, err := cont.Serve() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| sig := make(chan os.Signal, 1) | ||
| signal.Notify(sig, os.Interrupt, syscall.SIGINT, syscall.SIGTERM) | ||
|
|
||
| wg := &sync.WaitGroup{} | ||
| wg.Add(1) | ||
|
|
||
| stopCh := make(chan struct{}, 1) | ||
|
|
||
| go func() { | ||
| defer wg.Done() | ||
| for { | ||
| select { | ||
| case e := <-ch: | ||
| assert.Fail(t, "error", e.Error.Error()) | ||
| err = cont.Stop() | ||
| if err != nil { | ||
| assert.FailNow(t, "error", err.Error()) | ||
| } | ||
| case <-sig: | ||
| err = cont.Stop() | ||
| if err != nil { | ||
| assert.FailNow(t, "error", err.Error()) | ||
| } | ||
| return | ||
| case <-stopCh: | ||
| // timeout | ||
| err = cont.Stop() | ||
| if err != nil { | ||
| assert.FailNow(t, "error", err.Error()) | ||
| } | ||
| return | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| time.Sleep(time.Second * 3) | ||
| helpers.DisableProxy("redial", t) | ||
|
|
||
| go func() { | ||
| time.Sleep(time.Second * 2) | ||
| t.Run("PushPipelineWhileRedialing-1", helpers.PushToPipe("test-1", false, "127.0.0.1:6001")) | ||
| t.Run("PushPipelineWhileRedialing-2", helpers.PushToPipe("test-2", false, "127.0.0.1:6001")) | ||
| }() | ||
|
|
||
| time.Sleep(time.Second * 5) | ||
| helpers.EnableProxy("redial", t) | ||
| time.Sleep(time.Second * 2) | ||
|
|
||
| t.Run("PushPipelineWhileRedialing-1", helpers.PushToPipe("test-1", false, "127.0.0.1:6001")) | ||
| t.Run("PushPipelineWhileRedialing-2", helpers.PushToPipe("test-2", false, "127.0.0.1:6001")) | ||
|
|
||
| time.Sleep(time.Second * 10) | ||
| helpers.DestroyPipelines("127.0.0.1:6001", "test-1", "test-2") | ||
|
|
||
| stopCh <- struct{}{} | ||
| wg.Wait() | ||
| } |
There was a problem hiding this comment.
The test TestDurabilityBeanstalk is well structured and covers a good range of scenarios. However, there are some areas that could be improved for better maintainability and readability.
-
The use of
time.Sleep()to wait for certain operations or conditions can lead to flaky tests if the operation takes longer than expected. It would be better to use some form of synchronization or polling to wait for the condition to be met. -
The error handling within the goroutine (lines 74-99) could be simplified. Currently, the same error handling code is repeated three times. Consider refactoring this into a separate function.
-
The test cases "PushPipelineWhileRedialing-1" and "PushPipelineWhileRedialing-2" are run twice with the same parameters (lines 106-107 and 114-115). If the intention is to repeat the same test under different conditions, it would be clearer to encapsulate these conditions within the test case itself, rather than relying on the state of the system at the time the test case is run.
-
The test cleanup (line 118) should ideally be done in a
deferstatement immediately after the resources are created. This ensures that the cleanup is always performed, even if the test fails or panics before reaching the cleanup code.
Here's an example of how you might refactor the error handling:
func handleError(t *testing.T, cont *endure.Container, err error) {
assert.FailNow(t, "error", err.Error())
stopErr := cont.Stop()
if stopErr != nil {
assert.FailNow(t, "error", stopErr.Error())
}
}And then in your goroutine:
go func() {
defer wg.Done()
for {
select {
case e := <-ch:
handleError(t, cont, e.Error)
case <-sig:
handleError(t, cont, nil)
return
case <-stopCh:
// timeout
handleError(t, cont, nil)
return
}
}
}()| func EnableProxy(name string, t *testing.T) { | ||
| buf := new(bytes.Buffer) | ||
| buf.WriteString(`{"enabled":true}`) | ||
|
|
||
| resp, err := http.Post("http://127.0.0.1:8474/proxies/"+name, "application/json", buf) //nolint:noctx | ||
| require.NoError(t, err) | ||
| require.Equal(t, 200, resp.StatusCode) | ||
| if resp.Body != nil { | ||
| _ = resp.Body.Close() | ||
| } | ||
| } |
There was a problem hiding this comment.
This function enables a Toxiproxy proxy by sending an HTTP POST request with the payload {"enabled":true}. It asserts that no error occurred during the request and that the response status code is 200. If the response body is not nil, it is closed. The hardcoded URL and payload could be moved to constants or configuration files for better maintainability.
| func DisableProxy(name string, t *testing.T) { | ||
| buf := new(bytes.Buffer) | ||
| buf.WriteString(`{"enabled":false}`) | ||
|
|
||
| resp, err := http.Post("http://127.0.0.1:8474/proxies/"+name, "application/json", buf) //nolint:noctx | ||
| require.NoError(t, err) | ||
| require.Equal(t, 200, resp.StatusCode) | ||
| if resp.Body != nil { | ||
| _ = resp.Body.Close() | ||
| } | ||
| } |
There was a problem hiding this comment.
This function disables a Toxiproxy proxy by sending an HTTP POST request with the payload {"enabled":false}. It asserts that no error occurred during the request and that the response status code is 200. If the response body is not nil, it is closed. The hardcoded URL and payload could be moved to constants or configuration files for better maintainability.
| func DeleteProxy(name string, t *testing.T) { | ||
| client := &http.Client{} | ||
|
|
||
| req, err := http.NewRequest(http.MethodDelete, "http://127.0.0.1:8474/proxies/"+name, nil) //nolint:noctx | ||
| require.NoError(t, err) | ||
|
|
||
| resp, err := client.Do(req) | ||
| require.NoError(t, err) | ||
|
|
||
| require.NoError(t, err) | ||
| require.Equal(t, 204, resp.StatusCode) | ||
| if resp.Body != nil { | ||
| _ = resp.Body.Close() | ||
| } | ||
| } |
There was a problem hiding this comment.
This function deletes a Toxiproxy proxy by sending an HTTP DELETE request. It asserts that no error occurred during the request and that the response status code is 204. If the response body is not nil, it is closed. The hardcoded URL could be moved to constants or configuration files for better maintainability.
Signed-off-by: Valery Piashchynski <[email protected]>
Reason for This PR
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
EnableProxy,DisableProxy, andDeleteProxyin thehelperspackage to manage proxies during testing.TestDurabilityBeanstalkthat simulates network failures and tests the durability of the Beanstalk server.