-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
chore(slo): add api integration tests for main services #195316
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
eaaa689
to
beb8316
Compare
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
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.
Please don't use supertest
service to test API calls, as it will make API calls with operator privileges instead of native realm roles (viewer, editor, etc.)
{ sloId, slo }: { sloId: string; slo: UpdateSLOInput }, | ||
roleAuthc: RoleCredentials | ||
) { | ||
const { body } = await supertest |
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.
I just noticed that SloApiProvider
is using supertest
service. This service is authenticated with operator user and will use it to run API calls (ignoring roleAuthc.apiKeyHeader
). It is not a right approach, please update the service to supertestWithoutAuth
. This one has no authentication cached and will use provided API key / Cookie header to make API calls.
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, will do
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.
resolved in d23a04e
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.
@kdelemme We are actively working on migrating our current tests to the deployment agnostic framework. slo_api service in serverless folder will soon be removed. The slo_api service has already been migrated to the deployment agnostic folder and @dominiqueclarke is working into migrating the slo tests to the new framework.
.post(`/api/observability/slos/${sloId}/_reset`) | ||
.set(svlCommonApi.getInternalRequestHeader()) | ||
.set(roleAuthc.apiKeyHeader) | ||
.timeout(requestTimeout); |
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.
Feel free to ignore, just sharing another way to handle the wait:
If it makes sense to "fail fast" and not slow the test because of high request timeout (30s), the alternative is to set request retry count and decrease timeout. E.g. 10s request timeout and 2s delay before the next attempt
return await retry.tryWithRetries(
`Wait for SLO to be created`,
async () => {
const response = await supertestWithoutAuth
.get(`/api/observability/slos/${sloId}`)
.set(svlCommonApi.getInternalRequestHeader())
.set(roleAuthc.apiKeyHeader)
.timeout(10000);
if (response.body.id === undefined) {
throw new Error(`No slo with id ${sloId} found`);
}
return response.body;
},
{ retryCount: 5, retryDelay: 2000 }
);
Alerting API services uses it https://github.com/elastic/kibana/blob/main/x-pack/test/api_integration/deployment_agnostic/services/alerting_api.ts#L97-L130
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.
x-pack/test_serverless/api_integration/services
changes LGTM
💚 Build Succeeded
Metrics [docs]
History
|
Closing as outdated by #195927 |
resolves #187077
🌮 Summary
This PR adds the missing api integration tests on serverless for the update and reset SLO services.