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

FTR: enable ESLint mocha rules for api integration tests #191267

Merged
merged 21 commits into from
Aug 30, 2024

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Aug 26, 2024

Summary

Follow-up to #190690

Most of API integration tests does not match the path pattern set in the original PR (thanks @pheyos for catching it) and where not updated.
This PR updates .eslintrc.js with explicit patterns to lint api_integration tests. Hopefully it is final change, but I rely on code owners to double check it.

Most of the changes are trivial adjustments:

  • duplicated before/after hooks mocha/no-sibling-hooks
  • duplicated test titles mocha/no-identical-title
  • async function in describe() mocha/no-async-describe

@dmlemeshko dmlemeshko changed the title Ftr/eslint in api tests FTR: enable ESLint mocha rules for api integration tests Aug 26, 2024
Comment on lines -71 to -77
it('returns 404 error on non-existing scripted field', async () => {
const response1 = await supertest.get(
`/api/index_patterns/index_pattern/${indexPattern.id}/scripted_field/test`
);
expect(response1.status).to.be(404);
});

Copy link
Member Author

@dmlemeshko dmlemeshko Aug 26, 2024

Choose a reason for hiding this comment

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

Complete duplicate of

it('returns 404 error on non-existing scripted field', async () => {
const response1 = await supertest.get(
`/api/index_patterns/index_pattern/${indexPattern.id}/scripted_field/sf`
);
expect(response1.status).to.be(404);
});

in the same file

@@ -45,15 +45,15 @@ import type { ToolingLog } from '@kbn/tooling-log';
* @returns The response from the test
*/
export const retry = async <T>({
test,
testFn,
Copy link
Member Author

Choose a reason for hiding this comment

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

test is reserved by Mocha, renaming for convenience.

@@ -28,8 +28,6 @@ export default function ({ getService }: FtrProviderContext) {
},
};

after(async () => await deleteAllIndices());
Copy link
Member Author

Choose a reason for hiding this comment

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

duplicate hook:

after(async () => {
try {
await deleteAllIndices();
} catch (err) {
log.debug('[Cleanup error] Error deleting index');
throw err;
}
});

after(async () => {
await esArchiver.unload('x-pack/test/functional/es_archives/infra/metrics_and_logs');
await kibanaServer.savedObjects.cleanStandardList();
});
Copy link
Member Author

Choose a reason for hiding this comment

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

error Unexpected use of duplicate Mocha before hook mocha/no-sibling-hooks

Duplicating hooks is not recommended because it can lead to confusion about the order of execution and potential issues in test maintenance.

@@ -133,7 +130,7 @@ export default function (providerContext: FtrProviderContext) {
expect(downloadSource.host).to.eql('https://test.co:403');
});

it('should allow to update an existing download source', async function () {
it('should allow to update is_default for existing download source', async function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

error Test title is used multiple times in the same test suite mocha/no-identical-title

@dmlemeshko
Copy link
Member Author

/ci

Comment on lines 1355 to 1377
it('should discard the shipper values when shipper is disabled', async function () {
await supertest
.post(`/api/fleet/outputs`)
.set('kbn-xsrf', 'xxxx')
.send({
name: 'default monitoring output 1',
type: 'elasticsearch',
hosts: ['https://test.fr'],
is_default_monitoring: true,
shipper: {
disk_queue_enabled: true,
disk_queue_path: 'path/to/disk/queue',
disk_queue_encryption_enabled: true,
},
})
.expect(200);
const {
body: { items: outputs },
} = await supertest.get(`/api/fleet/outputs`).expect(200);
const defaultOutputs = outputs.filter((o: any) => o.is_default_monitoring);
expect(defaultOutputs[0].shipper).to.equal(null);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Duplicate of

it('should discard the shipper values when shipper is disabled', async function () {
await supertest
.post(`/api/fleet/outputs`)
.set('kbn-xsrf', 'xxxx')
.send({
name: 'default monitoring output 1',
type: 'elasticsearch',
hosts: ['https://test.fr'],
is_default_monitoring: true,
shipper: {
disk_queue_enabled: true,
disk_queue_path: 'path/to/disk/queue',
disk_queue_encryption_enabled: true,
},
})
.expect(200);
const {
body: { items: outputs },
} = await supertest.get(`/api/fleet/outputs`).expect(200);
const defaultOutputs = outputs.filter((o: any) => o.is_default_monitoring);
expect(defaultOutputs[0].shipper).to.equal(null);
});
in the same file

Comment on lines -491 to -505
it('should return nothing if searched for managed policy name', async () => {
const response = await supertest
.get(uninstallTokensRouteService.getListPath())
.query({
search: generatedManagedPolicyArray[0].name,
})
.expect(200);

const body: GetUninstallTokensMetadataResponse = response.body;
expect(body.total).to.equal(0);
expect(body.page).to.equal(1);
expect(body.perPage).to.equal(20);
expect(body.items).to.eql([]);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Duplicate of

it('should return nothing if searched for managed policy name', async () => {
const response = await supertest
.get(uninstallTokensRouteService.getListPath())
.query({
search: generatedManagedPolicyArray[0].name,
})
.expect(200);
const body: GetUninstallTokensMetadataResponse = response.body;
expect(body.total).to.equal(0);
expect(body.page).to.equal(1);
expect(body.perPage).to.equal(20);
expect(body.items).to.eql([]);
});
in the same file

@dmlemeshko
Copy link
Member Author

/ci

@dmlemeshko
Copy link
Member Author

/ci

1 similar comment
@dmlemeshko
Copy link
Member Author

/ci

dmlemeshko added a commit that referenced this pull request Aug 27, 2024
## Summary

This PR refactors the `setupFleetAndAgents` helper function into a
`FleetAndAgents` FTR service, which is now initialized during the config
file loading.

Reason: The previous implementation of `setupFleetAndAgents` wrapped a
Mocha `before` hook, which is considered an anti-pattern according to
the Mocha ESLint rules that the Operations and QA teams have agreed to
enable in the Kibana repo. It's best practice to avoid duplicating
hooks, as this can lead to inconsistent test results and make debugging
difficult. Ensuring a single before and after hook sequence guarantees
that the associated logic runs in a predictable and sequential manner.

This change also unblocks #191267, where we plan to enforce the same
recommended Mocha ESLint rules that were previously enabled for UI tests
in #190690.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@dmlemeshko dmlemeshko force-pushed the ftr/eslint-in-api-tests branch from 1946622 to 3705654 Compare August 28, 2024 08:32
@dmlemeshko
Copy link
Member Author

/ci

@dmlemeshko dmlemeshko added the release_note:skip Skip the PR/issue when compiling release notes label Aug 28, 2024
@botelastic botelastic bot added the Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team label Aug 28, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@pzl pzl requested review from ashokaditya and removed request for pzl August 28, 2024 17:43
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-presentation changes LGTM
code review only

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

ML code changes LGTM

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. security_solution_api_integration/edr_workflows changes LGTM.

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

kibana management changes lgtm

dmlemeshko and others added 2 commits August 29, 2024 10:38
…workflows/artifacts/trial_license_complete_tier/index.ts

Co-authored-by: Ash <1849116+ashokaditya@users.noreply.github.com>
@dmlemeshko dmlemeshko requested a review from pheyos August 29, 2024 09:03
Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Fleet changes

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

kibana-ci commented Aug 30, 2024

💛 Build succeeded, but was flaky

  • Buildkite Build
  • Commit: 3faef29
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-191267-3faef29f35e1

Failed CI Steps

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/test-suites-xpack 709 719 +10

Total ESLint disabled count

id before after diff
@kbn/test-suites-xpack 733 743 +10

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@delanni
Copy link
Contributor

delanni commented Aug 30, 2024

Admin-merging forgoing the missing few approvals

@delanni delanni merged commit 8436f45 into elastic:main Aug 30, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project FTR release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Obs AI Assistant Observability AI Assistant Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.