Refactor redis and mqtt tests#51699
Conversation
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThis change introduces shared Related PRs: None identified. Suggested labels: Suggested reviewers: None identified. Poem 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@filebeat/input/mqtt/testutil/testutil.go`:
- Around line 74-95: The failure handling in EmitMessages is unsafe because
require.Fail and require.NoError are called from the goroutine started by go
func(), which can leave the test hanging instead of failing cleanly. Update
EmitMessages to avoid require inside the background goroutine by using
assert-style checks or by sending publish errors back to the main test
goroutine; make sure the fix is applied at the Publish/WaitTimeout error
handling path and aligns with the downstream emitInputData/PublishMessage usage
pattern in the MQTT tests.
In `@filebeat/input/redis/testutil/testutil.go`:
- Around line 92-112: `EmitInputData` is calling `require.NoError` from inside
the background goroutine, which is unsafe because it can only stop that
goroutine and may panic after the test ends. Update the goroutine in
`testutil.EmitInputData` to use `assert.NoError` (or otherwise record the error
without `FailNow` semantics) for both `conn.Close()` and `conn.Do(...)`, while
keeping the surrounding `ticker`/`ctx.Done()` loop unchanged.
In `@x-pack/filebeat/tests/integration/otel_filebeat_input_test.go`:
- Around line 353-403: TestFilebeatOTelHTTPJSONInput starts a Beat instance
without matching cleanup, so add a deferred filebeat.Stop() right after
filebeat.Start() and register t.Cleanup to delete the data streams created by
the test using deleteDataStreamsFromES with the otelNamespace and fbNameSpace
patterns. Use the existing filebeat.Start(), filebeat.Stop(), and
deleteDataStreamsFromES helpers in this test so it matches the cleanup pattern
used by the other OTel integration tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 66961fe0-b4fa-45d0-bc7b-d3c698e82961
⛔ Files ignored due to path filters (3)
testing/environments/docker/redis/certs/root-ca.pemis excluded by!**/*.pemtesting/environments/docker/redis/certs/server-cert.pemis excluded by!**/*.pemtesting/environments/docker/redis/certs/server-key.pemis excluded by!**/*.pem
📒 Files selected for processing (12)
filebeat/docker-compose.ymlfilebeat/input/mqtt/mqtt_integration_test.gofilebeat/input/mqtt/testutil/testutil.gofilebeat/input/redis/_meta/Dockerfilefilebeat/input/redis/redis_integration_test.gofilebeat/input/redis/testutil/testutil.gotesting/environments/docker/redis/Dockerfiletesting/environments/docker/redis/Dockerfile-tlstesting/environments/docker/redis/envtesting/environments/docker/redis/redis.confx-pack/filebeat/tests/integration/otel_filebeat_input_test.gox-pack/otel/oteltestcol/collector.go
💤 Files with no reviewable changes (1)
- filebeat/input/redis/_meta/Dockerfile
This comment has been minimized.
This comment has been minimized.
TL;DR
Remediation
Investigation detailsRoot Cause
func NewWrapper(config *conf.C, r *mb.Register, info *beat.Info, monitoring beatmonitoring.Monitoring, p *paths.Path, options ...Option) (*Wrapper, error)But m, err := module.NewWrapper(config, mb.Registry, logptest.NewTestingLogger(t, ""), beatmonitoring.NewMonitoring(), paths.New(), module.WithMetricSetInfo())The same file already has the correct pattern at &beat.Info{Logger: logptest.NewTestingLogger(t, "")}Evidence
The PR changed 13 files under Filebeat Redis/MQTT test helpers and Docker setup; no Verification
Follow-upIf this gets fixed on What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
cmacknz
left a comment
There was a problem hiding this comment.
Fine besides comment on the healthcheck in the redis container potentially getting slower. Approving pre-emptively.
Proposed commit message
This PR moves some test functions into helper packages required for e2e testing for redis and mqtt inputs in otel mode
Related issues