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.

Sign up for GitHub

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

Jump to bottom

[FTR] enable recommended mocha + no-floating-promises ESLint rules #190690

Merged
merged 44 commits into from
Aug 23, 2024
Merged

[FTR] enable recommended mocha + no-floating-promises ESLint rules #190690

merged 44 commits into from
Aug 23, 2024

Conversation

dmlemeshko
Copy link
Member

@dmlemeshko dmlemeshko commented Aug 19, 2024 •
edited
Loading

Summary

This PR enforces ESLint rules in FTR tests, in particular:

  • no-floating-promises rule to catch unawaited Promises in tests/services/page objects

Why is it important?

  • Keep correct test execution order: cleanup code may run before the async operation is completed, leading to unexpected behavior in subsequent tests
  • Accurate test results: If a test completes before an async operation (e.g., API request) has finished, Mocha might report the test as passed or failed based on incomplete context.
198:11  error  Promises must be awaited, end with a call to .catch, end with a call to .then
with a rejection handler or be explicitly marked as ignored with the `void` operator 
@typescript-eslint/no-floating-promises
Screenshot 2024-08-20 at 14 04 43

Note for reviewers: some tests were skipped due to failures after missing await was added. Most likely is a "false positive" case when test is finished before async operation is actually completed. Please work on fixing and re-enabling it

@dmlemeshko
Copy link
Member Author

/ci

mistic
mistic reviewed Aug 19, 2024
View reviewed changes
.eslintrc.js Outdated Show resolved Hide resolved
@dmlemeshko dmlemeshko changed the title enable await rules for functional tests enable mocha eslint rules for functional tests Aug 19, 2024
@dmlemeshko
Copy link
Member Author

/ci

@dmlemeshko
Copy link
Member Author

/ci

@dmlemeshko
Copy link
Member Author

/ci

@dmlemeshko
Copy link
Member Author

/ci

@dmlemeshko dmlemeshko changed the title enable mocha eslint rules for functional tests [FTR] enable mocha eslint rules + no-floating-promises for tests Aug 20, 2024
@dmlemeshko dmlemeshko changed the title [FTR] enable mocha eslint rules + no-floating-promises for tests [FTR] enable mocha eslint rules + no-floating-promises Aug 20, 2024
@dmlemeshko
Copy link
Member Author

/ci

1 similar comment
@dmlemeshko
Copy link
Member Author

/ci

@dmlemeshko
Copy link
Member Author

/ci

afharo
afharo approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

This is awesome! Thanks for doing this! Core changes LGTM

wayneseymour
wayneseymour approved these changes Aug 23, 2024
View reviewed changes
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.

LGTM

mohamedhamed-ahmed
mohamedhamed-ahmed approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@mohamedhamed-ahmed mohamedhamed-ahmed 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, LGTM!

eokoneyo
eokoneyo approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@eokoneyo eokoneyo left a comment

Choose a reason for hiding this comment

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

Shared UX changes LGTM, thanks for this!!

walterra
walterra approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

ML related changes LGTM.

hop-dev
hop-dev approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@hop-dev hop-dev left a comment

Choose a reason for hiding this comment

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

Entity Analytics LGTM, thanks for this 🚀

cauemarcondes
cauemarcondes approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

nchaulet
nchaulet approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@nchaulet nchaulet 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 🚀

@pmuellr pmuellr mentioned this pull request Aug 23, 2024
Open
pmuellr
pmuellr approved these changes Aug 23, 2024
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

ResponseOps changes LGTM

I opened #191185 to track the skipped Cases test

pheyos
pheyos approved these changes Aug 23, 2024
View reviewed 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

Heenawter
Heenawter approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Code review - test changes LGTM 👍 Thanks so much for tackling this cleanup!

@dmlemeshko dmlemeshko enabled auto-merge (squash) August 23, 2024 16:07
rylnd
rylnd approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

Detection engine changes (which involve skipping a previously false-positive test) LGTM. Thanks!

@kibana-ci
Copy link
Collaborator

kibana-ci commented Aug 23, 2024 •
edited
Loading

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/core-application-browser-internal 1 3 +2
@kbn/test-suites-src 43 47 +4
total +6

Total ESLint disabled count

id before after diff
@kbn/core-application-browser-internal 1 3 +2
@kbn/test-suites-src 49 53 +4
total +6

History

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

cc @dmlemeshko

@dmlemeshko dmlemeshko merged commit fa69337 into elastic:main Aug 23, 2024
24 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 23, 2024
This was referenced Aug 26, 2024
Merged
Merged
dmlemeshko added a commit that referenced this pull request Aug 27, 2024
@dmlemeshko @kibanamachine
[ftr] add fleetAndAgents service (#191448)
069c158 
## 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 <[email protected]>
delanni pushed a commit that referenced this pull request Aug 30, 2024
@dmlemeshko @ashokaditya
FTR: enable ESLint mocha rules for api integration tests (#191267)
8436f45 
## 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`

---------

Co-authored-by: Ash <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rylnd rylnd rylnd approved these changes

@wayneseymour wayneseymour wayneseymour approved these changes

@jeramysoucy jeramysoucy jeramysoucy approved these changes

@cauemarcondes cauemarcondes cauemarcondes approved these changes

@davismcphee davismcphee davismcphee approved these changes

@opauloh opauloh opauloh approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

@maryam-saeidi maryam-saeidi maryam-saeidi approved these changes

@dominiqueclarke dominiqueclarke dominiqueclarke approved these changes

@mohamedhamed-ahmed mohamedhamed-ahmed mohamedhamed-ahmed approved these changes

@Heenawter Heenawter Heenawter approved these changes

@eokoneyo eokoneyo eokoneyo approved these changes

@afharo afharo afharo approved these changes

@pmuellr pmuellr pmuellr approved these changes

@mistic mistic mistic approved these changes

@hop-dev hop-dev hop-dev approved these changes

@TattdCodeMonkey TattdCodeMonkey TattdCodeMonkey approved these changes

@pheyos pheyos pheyos approved these changes

@adcoelho adcoelho adcoelho approved these changes

@nchaulet nchaulet nchaulet approved these changes

@sabarasaba sabarasaba sabarasaba approved these changes

@walterra walterra walterra approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

@delanni delanni Awaiting requested review from delanni

@machadoum machadoum Awaiting requested review from machadoum machadoum is a code owner automatically assigned from elastic/security-entity-analytics

@vitaliidm vitaliidm Awaiting requested review from vitaliidm vitaliidm is a code owner automatically assigned from elastic/security-detection-engine

Assignees

@dmlemeshko dmlemeshko

Labels
apm:review backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project es-lint 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 Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.