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

[Synthetics] migrate first set of tests #198950

Conversation

dominiqueclarke
Copy link
Contributor

@dominiqueclarke dominiqueclarke commented Nov 5, 2024

Summary

Relates to #196229

Migrates Synthetics tests to deployment agnostic

@dominiqueclarke
Copy link
Contributor Author

/ci

const { apiKey, isValid } = await getAPIKeyForSyntheticsService({ server: this.server });
if (!isValid) {
// do not check for api key validity if inspecting
if (!isValid && !inspect) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed when I was adjusting the inspect tests, that they would sometime fail on api key not valid. No need to check api key when doing an inspect.

import { getFixtureJson } from './helpers/get_fixture_json';
import { SyntheticsMonitorTestService } from '../../../services/synthetics_monitor';

// adjust the type of samlAuth
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized I left this as a comment for myself because samlAuth is set to any here. Didn't get to fixing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like to preface comments like this with TODO to signify that the PR has new tech debt to resolve before merging.

It's a nice reminder to the author themselves and it gives a much stronger indicator to reviewers that something is unfinished.

@dominiqueclarke
Copy link
Contributor Author

/ci

@dominiqueclarke
Copy link
Contributor Author

/ci

@dominiqueclarke
Copy link
Contributor Author

/ci

@dominiqueclarke
Copy link
Contributor Author

/ci

@dominiqueclarke
Copy link
Contributor Author

/ci

@dominiqueclarke
Copy link
Contributor Author

/ci

import { DeploymentAgnosticFtrProviderContext } from '../../../ftr_provider_context';

export default function ({ loadTestFile }: DeploymentAgnosticFtrProviderContext) {
describe('Synthetics', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
describe('Synthetics', () => {
describe('SyntheticsAPITests', () => {

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Dec 2, 2024
Copy link
Contributor

github-actions bot commented Dec 2, 2024

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !!

this.supertest = getService('supertestWithoutAuth');
this.samlAuth = getService('samlAuth');
this.getService = getService;
this.supertestWithoutAuth = getService('supertestWithoutAuth');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First time I've seen supertestWithoutAuth imported twice.
What's the deal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No deal. It's a mistake. Thanks for the catch!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's still imported twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import { KibanaSupertestProvider } from '@kbn/ftr-common-functional-services';
import { DeploymentAgnosticFtrProviderContext } from '../ftr_provider_context';

// fix saml auth type
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I fixed this. I'll have to adjust.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want this comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also outdated. I believe you're looking at an odd commit.

@pheyos
Copy link
Member

pheyos commented Dec 4, 2024

@dominiqueclarke I gave this a run against a real serverless project in MKI and I see many test failures: https://buildkite.com/elastic/appex-qa-serverless-kibana-ftr-tests/builds/3519.
Deployment agnostic tests should also pass when running against MKI projects and ESS deployments.

@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 11, 2024

💚 Build Succeeded

  • Buildkite Build
  • Commit: 7bce1ca
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-198950-7bce1ca2d862

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/test-suites-xpack 727 729 +2

Total ESLint disabled count

id before after diff
@kbn/test-suites-xpack 752 754 +2

History

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question.

this.supertest = getService('supertestWithoutAuth');
this.samlAuth = getService('samlAuth');
this.getService = getService;
this.supertestWithoutAuth = getService('supertestWithoutAuth');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's still imported twice?

Copy link
Member

@wayneseymour wayneseymour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CR only, LGTM

@dominiqueclarke dominiqueclarke merged commit b74b935 into elastic:main Dec 12, 2024
8 checks passed
@dominiqueclarke dominiqueclarke deleted the chore/synthetics-deployment-agnostic branch December 12, 2024 14:41
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.17, 8.x

https://github.com/elastic/kibana/actions/runs/12298691592

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.17 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.17:
- [UA] Remove 7->8 multitype check (#203995)
- [UA] kibana-core ownership (#203410)
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 198950

Questions ?

Please refer to the Backport tool documentation

@dominiqueclarke
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x
8.17

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

dominiqueclarke added a commit to dominiqueclarke/kibana that referenced this pull request Dec 12, 2024
## Summary

Relates to elastic#196229

Migrates Synthetics tests to deployment agnostic

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Shahzad <[email protected]>
(cherry picked from commit b74b935)

# Conflicts:
#	.github/CODEOWNERS
#	x-pack/test/api_integration/deployment_agnostic/default_configs/serverless.config.base.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) chore ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants