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

[ML] Functional tests - fix and re-enable module API tests #102477

Merged
merged 2 commits into from
Jun 17, 2021
Merged

[ML] Functional tests - fix and re-enable module API tests #102477

merged 2 commits into from
Jun 17, 2021

Conversation

pheyos
Copy link
Member

@pheyos pheyos commented Jun 17, 2021

Summary

This PR fixes and re enables the temporarily skipped API test suite for ML modules.

Details

  • In the past we used a static Fleet package version, which worked for a while.
  • Then it started failing with [error][fleet][plugins] apache-0.5.0 is out-of-date and cannot be installed or updated.
  • With this PR we're no longer installing Fleet packages with a static version. Instead we're grabbing the available package version from the API and use that one.

Closes #102282
Closes #102283

@pheyos pheyos self-assigned this Jun 17, 2021
@pheyos pheyos requested a review from a team as a code owner June 17, 2021 09:23
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

peteharverson
peteharverson approved these changes Jun 17, 2021
View reviewed changes
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.

LGTM

jgowdyelastic
jgowdyelastic reviewed Jun 17, 2021
View reviewed changes
x-pack/test/functional/services/ml/test_resources.ts Outdated
Comment on lines 573 to 581
for (const packageDefinition of body.response) {
if (packageDefinition.name === packageName) {
if (packageDefinition.version) {
log.debug(` > found version '${packageDefinition.version}'`);
packageVersion = packageDefinition.version as string;
break;
}
}
}
Copy link
Member

@jgowdyelastic jgowdyelastic Jun 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nit, this could be written with less code.

for (const { name, version } of body.response) {
   if (name === packageName && version) {
     log.debug(` > found version '${version}'`);
     packageVersion = version as string;
     break;
  }
}

or even something like

const packageVersion = body.response.find(({name, version}) => name === packageName && version)?.version ?? ''

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in aac44cc

jgowdyelastic
jgowdyelastic approved these changes Jun 17, 2021
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @pheyos

@pheyos pheyos added the auto-backport Deprecated - use backport:version if exact versions are needed label Jun 17, 2021
@pheyos pheyos merged commit a9d7431 into elastic:master Jun 17, 2021
@pheyos pheyos deleted the fix_package_module_tests branch June 17, 2021 14:13
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 17, 2021
@pheyos @kibanamachine
[ML] Functional tests - fix and re-enable module API tests (elastic#1…
7cc7ab4 
…02477)

This PR fixes and re-enables the temporarily skipped API test suite for ML modules.
@kibanamachine kibanamachine mentioned this pull request Jun 17, 2021
Merged
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
❌ 7.13 Commit could not be cherrypicked due to conflicts
✅ 7.x

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 102477

@pheyos pheyos mentioned this pull request Jun 17, 2021
Merged
kibanamachine added a commit that referenced this pull request Jun 17, 2021
@kibanamachine @pheyos
[ML] Functional tests - fix and re-enable module API tests (#102477) (#…
e27b86f 
…102510)

This PR fixes and re-enables the temporarily skipped API test suite for ML modules.

Co-authored-by: Robert Oskamp <[email protected]>
pheyos added a commit that referenced this pull request Jun 17, 2021
@pheyos
[ML] Functional tests - fix and re-enable module API tests (#102477) (#…
fd5f62d 
…102515)

This PR fixes and re-enables the temporarily skipped API test suite for ML modules.
# Conflicts:
#	x-pack/test/functional/services/ml/test_resources.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

@peteharverson peteharverson peteharverson approved these changes

Assignees

@pheyos pheyos

Labels
auto-backport Deprecated - use backport:version if exact versions are needed :ml release_note:skip Skip the PR/issue when compiling release notes test_ui_functional v7.13.3 v7.14.0 v8.0.0
Projects
None yet
Milestone
No milestone
5 participants
@pheyos @elasticmachine @kibanamachine @peteharverson @jgowdyelastic