-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[cmd/opampsupervisor] Add integration/E2E tests #27002
[cmd/opampsupervisor] Add integration/E2E tests #27002
Conversation
c0e0537
to
8190ff2
Compare
@evan-bradley ping me when this is ready for review. |
773fea1
to
2b6afc7
Compare
@tigrannajaryan The tests are passing in CI now, this is ready for review. Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these @evan-bradley, it is great to have e2e tests for the supervisor.
I left some comments/questions to help me understand how the tests work.
6fa78c3
to
8475a4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one minor comment.
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
8f2a2ee
to
f14a4a6
Compare
@atoulme Could you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing jumps at me as worthy of pushback, it's great to see more testing. LGTM
f52646b
to
3e7e1d0
Compare
Description:
Add tests that verify the Supervisor's behavior using a real Collector binary against an OpAMP server written with the opamp-go library. This tests a basic set of functionality mostly based on what is currently listed as implemented in the Supervisor's feature table.
Right now these tests start the Supervisor using the Go API to avoid process handling, but could be changed to start it through a binary in the future. I've placed these tests in the E2E testing workflow even though they run fairly quickly since they depend on a built Collector binary, may become more expansive in the future, and don't fit in any of the existing jobs in the
build-and-test
workflow. I'm open to placing them in another location if we'd prefer them elsewhere.This also updates opamp-go to v0.9.0 to take advantage of open-telemetry/opamp-go#200.
Link to tracking Issue:
Resolves #24292