Skip to content
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.

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

fix(loadt): Fix a bug that overwrote the original error #1143

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

mi-wada
Copy link
Contributor

@mi-wada mi-wada commented Jan 8, 2025

What I did in this PR

Currently at runn loadt cmd, the existing error (err) was overwritten by the new error returned from runn.RemoveCacheDir(). This change uses errors.Join(err, ...) to merge both errors and ensure the original context is not lost.

Background

By this bug, even if the condition of --threshold flag does not meets, no error was returned (exit with 0).

before

$ go run cmd/runn/main.go loadt --env-file load_test/.env load_test/main.yaml --duration 1s --warm-up 0s --threshold 'total > 10'

Number of runbooks per RunN....: 1
Warm up time (--warm-up).......: 0s
Duration (--duration)..........: 1s
Concurrent (--load-concurrent).: 1
Max RunN per second (--max-rps): 1

Total..........................: 2
Succeeded......................: 2
Failed.........................: 0
Error rate.....................: 0%
RunN per second................: 2
Latency .......................: max=21.0ms min=11.2ms avg=16.1ms med=11.2ms p(90)=11.2ms p(99)=11.2ms

after

$ go run cmd/runn/main.go loadt --env-file load_test/.env load_test/main.yaml --duration 1s --warm-up 0s --threshold 'total > 10'

Number of runbooks per RunN....: 1
Warm up time (--warm-up).......: 0s
Duration (--duration)..........: 1s
Concurrent (--load-concurrent).: 1
Max RunN per second (--max-rps): 1

Total..........................: 2
Succeeded......................: 2
Failed.........................: 0
Error rate.....................: 0%
RunN per second................: 2
Latency .......................: max=54.4ms min=16.0ms avg=35.2ms med=16.0ms p(90)=16.0ms p(99)=16.0ms

Error: (total > 10) is not true
total > 10

├── total => 2
└── 10

exit status 1

Previously, the existing error (`err`) was overwritten by the new error returned from `runn.RemoveCacheDir()`. This change uses `errors.Join(err, ...)` to merge both errors and ensure the original context is not lost.

As a result of this bug, even if the operation failed when the `--threshold` flag was passed, no error was returned (exit with 0).
@k1LoW k1LoW added the bug Something isn't working label Jan 8, 2025
@k1LoW
Copy link
Owner

k1LoW commented Jan 8, 2025

@mi-wada Thank you!!

@k1LoW k1LoW merged commit 013c95d into k1LoW:main Jan 8, 2025
7 checks passed
@mi-wada mi-wada deleted the fix-override-err branch January 8, 2025 07:01
@github-actions github-actions bot mentioned this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants