chore: tests with Go workspaces #90
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
WalkthroughThis pull request introduces a comprehensive suite of changes, primarily focusing on testing and configuration. It adds GitHub Actions workflows for continuous integration, modifies .gitignore, and includes numerous YAML configuration files. The PR also introduces helper functions for testing, mock logger implementations, and PHP scripts for job handling. Changes
TipsChat with CodeRabbit Bot (
|
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
![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: 27
Files ignored due to filter (10)
- go.mod
- go.sum
- go.work
- go.work.sum
- tests/env/docker-compose-nats.yaml
- tests/env/otel-collector-config.yml
- tests/go.mod
- tests/go.sum
- tests/php_test_files/composer.json
- tests/php_test_files/composer.lock
Files selected for processing (31)
- .github/linux.yml (1 hunks)
- .github/linux_durability.yml (1 hunks)
- .github/workflows/linters.yml (1 hunks)
- .gitignore (1 hunks)
- tests/configs/.rr-nats-declare.yaml (1 hunks)
- tests/configs/.rr-nats-durability-redial.yaml (1 hunks)
- tests/configs/.rr-nats-init-v27-br.yaml (1 hunks)
- tests/configs/.rr-nats-init-v27.yaml (1 hunks)
- tests/configs/.rr-nats-init.yaml (1 hunks)
- tests/configs/.rr-nats-jobs-err.yaml (1 hunks)
- tests/configs/.rr-nats-otel.yaml (1 hunks)
- tests/configs/.rr-nats-pq.yaml (1 hunks)
- tests/configs/.rr-nats-raw.yaml (1 hunks)
- tests/configs/.rr-nats-stat.yaml (1 hunks)
- tests/configs/.rr-no-global.yaml (1 hunks)
- tests/helpers/helpers.go (1 hunks)
- tests/jobs_nats_durability_test.go (1 hunks)
- tests/jobs_nats_test.go (1 hunks)
- tests/mock/logger.go (1 hunks)
- tests/mock/observer.go (1 hunks)
- tests/php_test_files/jobs/jobs_bad_resp.php (1 hunks)
- tests/php_test_files/jobs/jobs_create_memory.php (1 hunks)
- tests/php_test_files/jobs/jobs_err.php (1 hunks)
- tests/php_test_files/jobs/jobs_ok.php (1 hunks)
- tests/php_test_files/jobs/jobs_ok_pq.php (1 hunks)
- tests/php_test_files/jobs/jobs_ok_queue_name_exist.php (1 hunks)
- tests/php_test_files/jobs/jobs_ok_sleep1.php (1 hunks)
- tests/php_test_files/jobs/jobs_ok_slow.php (1 hunks)
- tests/php_test_files/jobs/jobs_ok_slow_rand.php (1 hunks)
- tests/php_test_files/jobs/jobs_send.php (1 hunks)
- tests/pkgs.txt (1 hunks)
Files skipped from review due to trivial changes (13)
- .github/linux.yml
- .github/workflows/linters.yml
- .gitignore
- tests/configs/.rr-nats-declare.yaml
- tests/configs/.rr-nats-init-v27-br.yaml
- tests/configs/.rr-nats-otel.yaml
- tests/configs/.rr-nats-raw.yaml
- tests/configs/.rr-nats-stat.yaml
- tests/configs/.rr-no-global.yaml
- tests/php_test_files/jobs/jobs_ok.php
- tests/php_test_files/jobs/jobs_ok_sleep1.php
- tests/pkgs.txt
- tests/jobs_nats_test.go (diff too large)
Additional comments (Suppressed): 18
tests/configs/.rr-nats-init.yaml (1)
- 1-50: The configuration file seems to be well structured and follows the YAML syntax correctly. It specifies settings for RPC, server command, NATS messaging, logging, and job pipelines. However, please ensure that the specified ports (e.g., 6001 for RPC, 4222 for NATS) are open and not being used by other services. Also, verify that the PHP script
php_test_files/jobs/jobs_ok.phpexists and is executable. Lastly, make sure that the NATS subjects and streams ("default", "foo", "default-2", "foo-2") are correctly set up in your NATS server.tests/configs/.rr-nats-init-v27.yaml (1)
- 1-48: The configuration file seems to be well structured and follows the YAML syntax correctly. It specifies settings for RPC, server command, NATS messaging, logging, and job pipelines. However, please ensure that the specified ports (6001 for RPC and 4222 for NATS) are open and not being used by other services. Also, verify that the PHP script
php_test_files/jobs/jobs_ok.phpexists and is executable. Lastly, confirm that the subjects and streams ("default-3", "foo-3", "default-4", "foo-4") are correctly set up in your NATS server.tests/configs/.rr-nats-durability-redial.yaml (1)
- 1-55: The configuration file seems to be well structured and follows the YAML syntax correctly. It includes configurations for RPC, server command, NATS messaging, logging, and job pipelines which are all crucial for the system under test. However, please ensure that the specified ports (e.g., 6001 for RPC, 19224 for NATS) are open and not being used by other services. Also, verify that the PHP script
php_test_files/jobs/jobs_ok.phpexists and is executable. Lastly, make sure that the NATS subjects and streams (default,foo,default2,foo2) are properly set up in your NATS server.tests/configs/.rr-nats-jobs-err.yaml (1)
- 1-26: The configuration file
tests/configs/.rr-nats-jobs-err.yamlseems to be well structured and follows the YAML syntax correctly. It specifies settings for RPC, server command, NATS messaging, job pipelines, and logging. However, please ensure that the specified ports (6001 for RPC and 4222 for NATS) are open and not being used by other services. Also, verify that the PHP scriptphp_test_files/jobs/jobs_err.phpexists and is executable. Lastly, make sure that the log level, encoding, and mode are appropriate for your testing environment.tests/configs/.rr-nats-pq.yaml (1)
- 1-48: The configuration file seems to be well structured and follows the YAML syntax correctly. It specifies settings for RPC, server command, NATS messaging, logging, job pipelines, and other parameters required for the system under test. However, please ensure that the specified ports (6601 for RPC and 4222 for NATS) are open and not being used by any other services. Also, verify that the PHP script
php_test_files/jobs/jobs_ok_pq.phpexists at the specified path and has the correct permissions to be executed. Lastly, confirm that the subjects and streams ("default-pq", "foo-pq", "default-2-pq", "foo-2-pq") are correctly set up in your NATS server.tests/php_test_files/jobs/jobs_bad_resp.php (1)
- 16-22: This PHP script is designed to handle jobs with RoadRunner. However, it always responds with a static payload 'foo' regardless of the input job. This might not be an issue if this script is only used for testing purposes and you want to simulate a specific behavior. But if it's intended for more general use, you should consider processing the input
$inand generating a response based on that.while ($in = $rr->waitPayload()) { try { - $rr->respond(new RoadRunner\Payload('foo')); + // Process $in and generate appropriate response + $response = processJob($in); + $rr->respond(new RoadRunner\Payload($response)); } catch (\Throwable $e) { $rr->error((string)$e); } }Please verify whether this static response is intentional or not.
tests/mock/observer.go (1)
- 1-199: The mock logger implementation seems to be well designed and follows good practices. It provides a way to observe logs in a concurrency-safe manner, which is crucial for testing multi-threaded code. The use of
sync.RWMutexensures that multiple readers can access the log entries concurrently, while writers have exclusive access. This is a good practice for performance reasons.The various filter methods provided (
FilterLevelExact,FilterMessage,FilterMessageSnippet,FilterField,FilterFieldKey) are useful for narrowing down the log entries based on specific criteria. This will be helpful in tests where you want to assert that certain log messages were generated.The
contextObserverstruct and its methods implement thezapcore.Coreinterface, allowing it to be used as a drop-in replacement for a real logger in tests. This is a good example of the Dependency Inversion Principle (part of SOLID principles), where high-level modules (tests) depend on abstractions (zapcore.Core), not on low-level details (specific logger implementations).One thing to note is that the
contextObserver.Writemethod does not return any errors. While this is acceptable in a mock implementation used for testing, in a real logger, error handling would be necessary to deal with potential issues such as disk full, permission denied, etc.Overall, this mock logger should serve well for testing logging behavior without side effects.
tests/php_test_files/jobs/jobs_ok_slow_rand.php (1)
- 16-16: The
Consumerclass is being instantiated without any arguments. However, according to the RoadRunner documentation, theConsumerconstructor requires aRelayInterfaceinstance as an argument. Please verify this and update the code accordingly.- $consumer = new Spiral\RoadRunner\Jobs\Consumer(); + $relay = new Goridge\StreamRelay(STDIN, STDOUT); + $consumer = new Spiral\RoadRunner\Jobs\Consumer($relay);tests/php_test_files/jobs/jobs_create_memory.php (3)
13-19: The code creates a new Jobs service and a queue with the name 'example', priority 10, and prefetch 100. It then resumes the queue. Ensure that the server at 'tcp://127.0.0.1:6001' is up and running before this script is executed. Also, verify if the parameters for the
MemoryCreateInfoare appropriate for your use case.24-29: A task named 'ping' with payload '{"site": "https://example.com"}' is created and dispatched to the 'example' queue. The task has headers specifying 4 attempts and a retry delay of 10. Make sure these values are suitable for your requirements.
31-41: The script connects to a 'local' pipeline from jobs, resumes it, creates another 'ping' task with the same payload and headers as before, and dispatches it to the 'local' queue. Again, ensure that these settings align with your needs.
.github/linux_durability.yml (3)
70-70: The
-raceflag is used when running the tests, which is good for detecting race conditions. However, it can significantly slow down the tests. If the performance of the tests becomes an issue, consider removing this flag or only using it in certain situations.73-77: The code coverage results are being archived as an artifact. This is a good practice as it allows for easier access to these results later on.
79-99: The codecov job depends on the
nats_durability_testjob. This ensures that the codecov job will only run if the tests pass, which is a good practice. The coverage results are then downloaded, combined into a single file, and uploaded to codecov. This is a good way to track code coverage over time.tests/php_test_files/jobs/jobs_send.php (1)
- 1-23: This PHP script is used to send jobs using the Spiral RoadRunner Jobs package. It connects to a job queue named 'test-1' and dispatches a job with the name 'my-name', payload
['foo' => 'bar'], and Kafka options specifying the topic as 'mytopic' and the offset as newest.Ensure that the server at 'tcp://127.0.0.1:6001' is up and running before executing this script, and that the Kafka topic 'mytopic' exists. Also, verify that the job queue 'test-1' is properly configured and ready to receive jobs.
- $jobs = new Jobs(RPC::create('tcp://127.0.0.1:6001')); + $rpcAddress = getenv('RPC_ADDRESS') ?: 'tcp://127.0.0.1:6001'; + $jobs = new Jobs(RPC::create($rpcAddress));The above change will allow you to configure the RPC address via an environment variable, which can be useful for testing in different environments.
tests/mock/logger.go (3)
17-24: The
ZapTestLoggerfunction creates a newZapLoggerMockandObservedLogsusing a providedzapcore.LevelEnabler. It's not clear where theNewfunction called on line 18 is defined or what it does. Ensure that this function is properly defined and imported, and that it returns the expected types.38-42: The
Providesmethod returns a slice of pointers todep.Outobjects, which bind theLoggerinterface to theProvideLoggermethod. This seems to be part of the dependency injection setup for the logger. Ensure that this setup is correct and that theProvideLoggermethod returns a value that satisfies theLoggerinterface.48-50: The
ProvideLoggermethod creates a newLogobject with thezap.Loggerfrom theZapLoggerMock. Ensure that this logger has been properly initialized before this method is called.
| ini_set('display_errors', 'stderr'); | ||
| require dirname(__DIR__) . "/vendor/autoload.php"; | ||
|
|
||
| $consumer = new Spiral\RoadRunner\Jobs\Consumer(); |
There was a problem hiding this comment.
The Consumer class requires a RelayInterface instance as an argument in its constructor. However, it's being instantiated without any arguments which will lead to an error.
- $consumer = new Spiral\RoadRunner\Jobs\Consumer();
+ $relay = new Goridge\StreamRelay(STDIN, STDOUT);
+ $consumer = new Spiral\RoadRunner\Jobs\Consumer($relay);| $task->withHeader("attempts",$total_attempts)->withDelay(5)->fail("failed", true); | ||
| } | ||
| } catch (\Throwable $e) { | ||
| $rr->error((string)$e); |
There was a problem hiding this comment.
The variable $rr is not defined anywhere in the code before it's used. This will cause an undefined variable error. You might want to instantiate a RoadRunner object and assign it to $rr.
- $rr->error((string)$e);
+ $worker = RoadRunner\Worker::create();
+ $worker->error((string)$e);|
|
||
| $task->complete(); | ||
| } catch (\Throwable $e) { | ||
| $rr->error((string)$e); |
There was a problem hiding this comment.
The variable $rr is not defined in the current scope. It seems like you're trying to log an error message using the RoadRunner worker instance. You should create an instance of RoadRunner\Worker and use it for logging.
- $rr->error((string)$e);
+ $worker = RoadRunner\Worker::create();
+ $worker->error((string)$e);| ini_set('display_errors', 'stderr'); | ||
| require dirname(__DIR__) . "/vendor/autoload.php"; | ||
|
|
||
| $consumer = new Spiral\RoadRunner\Jobs\Consumer(); |
There was a problem hiding this comment.
The Consumer class requires a RelayInterface instance as an argument in its constructor. However, it's being instantiated without any arguments which will lead to a runtime error.
- $consumer = new Spiral\RoadRunner\Jobs\Consumer();
+ $relay = new Goridge\StreamRelay(STDIN, STDOUT);
+ $consumer = new Spiral\RoadRunner\Jobs\Consumer($relay);
tests/helpers/helpers.go
Outdated
| conn, err := net.Dial("tcp", "127.0.0.1:6001") | ||
| assert.NoError(t, err) | ||
| c := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn)) | ||
|
|
||
| var ret bool | ||
| err = c.Call("resetter.Reset", "jobs", &ret) | ||
| assert.NoError(t, err) | ||
| require.True(t, ret) | ||
| } |
There was a problem hiding this comment.
The Reset function should close the connection after use.
conn, err := net.Dial("tcp", "127.0.0.1:6001")
assert.NoError(t, err)
+ defer conn.Close()
c := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn))
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files ignored due to filter (4)
- go.mod
- go.sum
- tests/go.mod
- tests/go.sum
Files selected for processing (2)
- .github/workflows/linux.yml (1 hunks)
- .github/workflows/linux_durability.yml (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/linux.yml
- .github/workflows/linux_durability.yml
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Welcome to Codecov 🎉Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment. Thanks for integrating Codecov - We've got you covered ☂️ |
| er := &jobsProto.Empty{} | ||
| err = client.Call(pause, pipe, er) | ||
| assert.NoError(t, err) | ||
| } | ||
| } | ||
|
|
||
| func DestroyPipelines(address string, pipes ...string) func(t *testing.T) { | ||
| return func(t *testing.T) { | ||
| conn, err := net.Dial("tcp", address) | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| _ = conn.Close() | ||
| }() | ||
| client := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn)) | ||
|
|
||
| pipe := &jobsProto.Pipelines{Pipelines: make([]string, len(pipes))} | ||
|
|
||
| for i := 0; i < len(pipes); i++ { | ||
| pipe.GetPipelines()[i] = pipes[i] | ||
| } | ||
|
|
||
| for i := 0; i < 10; i++ { | ||
| er := &jobsProto.Empty{} | ||
| err = client.Call(destroy, pipe, er) | ||
| if err != nil { | ||
| time.Sleep(time.Second) | ||
| continue | ||
| } | ||
| assert.NoError(t, err) | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func Stats(address string, state *jobState.State) func(t *testing.T) { | ||
| return func(t *testing.T) { | ||
| conn, err := net.Dial("tcp", address) | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| _ = conn.Close() | ||
| }() | ||
| client := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn)) | ||
|
|
||
| st := &jobsProto.Stats{} | ||
| er := &jobsProto.Empty{} | ||
|
|
||
| err = client.Call(stat, er, st) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, st) | ||
|
|
||
| state.Queue = st.Stats[0].Queue | ||
| state.Pipeline = st.Stats[0].Pipeline | ||
| state.Driver = st.Stats[0].Driver | ||
| state.Active = st.Stats[0].Active | ||
| state.Delayed = st.Stats[0].Delayed | ||
| state.Reserved = st.Stats[0].Reserved | ||
| state.Ready = st.Stats[0].Ready | ||
| state.Priority = st.Stats[0].Priority | ||
| } | ||
| } | ||
|
|
||
| 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() | ||
| } | ||
| } | ||
|
|
||
| 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() | ||
| } | ||
| } | ||
|
|
||
| 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.
The helper functions in this file are well-written and follow good practices. They handle errors properly, use the require package to fail tests early when necessary, and close resources such as network connections and HTTP response bodies in defer statements to ensure they are always closed even if an error occurs.
However, there is a repeated pattern of creating a TCP connection, initializing an RPC client, and making an RPC call. This could be abstracted into a separate function to reduce code duplication and improve maintainability. Here's an example of how you might do that:
func withRPCClient(address string, fn func(client *rpc.Client)) func(t *testing.T) {
return func(t *testing.T) {
conn, err := net.Dial("tcp", address)
require.NoError(t, err)
defer func() {
_ = conn.Close()
}()
client := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn))
fn(client)
}
}Then, you can use this function in your test helpers like so:
- func ResumePipes(address string, pipes ...string) func(t *testing.T) {
+ func ResumePipes(address string, pipes ...string) {
return withRPCClient(address, func(client *rpc.Client) {
pipe := &jobsProto.Pipelines{Pipelines: make([]string, len(pipes))}
for i := 0; i < len(pipes); i++ {
pipe.GetPipelines()[i] = pipes[i]
}
er := &jobsProto.Empty{}
err := client.Call(resume, pipe, er)
require.NoError(t, err)
})
}This change will make your code more DRY (Don't Repeat Yourself), which is a key principle of clean and maintainable code.
There was a problem hiding this comment.
You missed t *testing.T as in arg
There was a problem hiding this comment.
@rustatian You're correct, my apologies for the oversight. The t *testing.T argument is necessary in the function signature to allow the use of testing assertions within the function. Here's the corrected version:
func withRPCClient(t *testing.T, address string, fn func(client *rpc.Client)) {
conn, err := net.Dial("tcp", address)
require.NoError(t, err)
defer func() {
_ = conn.Close()
}()
client := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn))
fn(client)
}And here's how you would use it in your test helpers:
- func ResumePipes(address string, pipes ...string) func(t *testing.T) {
+ func ResumePipes(t *testing.T, address string, pipes ...string) {
withRPCClient(t, address, func(client *rpc.Client) {
pipe := &jobsProto.Pipelines{Pipelines: make([]string, len(pipes))}
for i := 0; i < len(pipes); i++ {
pipe.GetPipelines()[i] = pipes[i]
}
er := &jobsProto.Empty{}
err := client.Call(resume, pipe, er)
require.NoError(t, err)
})
}Thank you for pointing out the mistake.
There was a problem hiding this comment.
You also changed the return type from func(t *testing.T) to void. It is not possible to use this function with t.Run anymore.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 29 minutes and 59 seconds before requesting another review. To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR. |
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Rate Limit ExceededYou have exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 29 minutes and 59 seconds before requesting another review. To re-trigger a review, convert this PR to a draft and then mark it as ready for review again. Alternatively, you can push a new commit to this PR. |
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
helperswith various helper functions to facilitate testing..gitignorefile to include thevendordirectory.