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] API integration tests for UPDATE data frame analytics endpoint #72710

Merged
merged 4 commits into from
Jul 22, 2020
Merged

[ML] API integration tests for UPDATE data frame analytics endpoint #72710

merged 4 commits into from
Jul 22, 2020

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Jul 21, 2020 •
edited
Loading

Summary

Part of #70354

Adds API integration tests for:

  • /api/ml/data_frame/analytics/{analyticsId}/_update

Checklist

Delete any items that are not applicable to this PR.

@alvarezmelissa87 alvarezmelissa87 added :ml release_note:skip Skip the PR/issue when compiling release notes test-api-integration Feature:Data Frame Analytics ML data frame analytics features v7.10.0 labels Jul 21, 2020
@alvarezmelissa87 alvarezmelissa87 self-assigned this Jul 21, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Jul 21, 2020
Closed
12 tasks
@alvarezmelissa87
Copy link
Contributor Author

retest

peteharverson
peteharverson approved these changes Jul 22, 2020
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

pheyos
pheyos reviewed Jul 22, 2020
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good overall, thanks for adding the tests @alvarezmelissa87 !
Just left two small suggestions in the code. And also one general question / suggestion:
You're currently validating the updated fields based on details in the response body. Are we sure we can to trust the response body in these tests? I'd suggest to GET the DFA job again with a separate request and validate based on this one to make sure the fields have really changed.

@pheyos - I think we can trust the response but I've gone ahead and fetched the job separately to make sure defc6bd

@alvarezmelissa87 - great, thanks!

x-pack/test/api_integration/apis/ml/data_frame_analytics/update.ts Show resolved Hide resolved
x-pack/test/api_integration/apis/ml/data_frame_analytics/update.ts Show resolved Hide resolved
@alvarezmelissa87
Copy link
Contributor Author

This has been updated and is ready for a final look when you get a chance cc @pheyos, @peteharverson 🙏

qn895
qn895 reviewed Jul 22, 2020
View reviewed changes
x-pack/test/api_integration/apis/ml/data_frame_analytics/update.ts

const fetchedJob = await getDFAJob(analyticsId);
// Description should not have changed
expect(fetchedJob.description).to.eql(editedDescription);
Copy link
Member

@qn895 qn895 Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

At first glance it would be hard to know fetchedJob.description should equal editedDescription based on the variable name, even though I know we already made a request to update the job to that editedDescription earlier. I wonder if it would be clearer to expect fetchedJob.description to not eql requestBody.description

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 think I prefer to have an explicit check for the description so we know it hasn't been changed in any way. A not equal check could potentially miss any undesired changes to the description.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @alvarezmelissa87 - we should stay with the explicit checks. The only thing that comes to my mind for making it more clear, is to maybe rename fetchedJob to updatedJob?

@qn895
Copy link
Member

qn895 commented Jul 22, 2020

LGTM aside from my one minor comment.

pheyos
pheyos approved these changes Jul 22, 2020
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 🎉

@pheyos
Copy link
Member

pheyos commented Jul 22, 2020 •
edited
Loading

Checking test stability in a flaky test runner job ...
=> No ML failure in 50 runs ✔️ (just one unrelated Ingest Node Pipelines failure)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@alvarezmelissa87 alvarezmelissa87 merged commit 1889c68 into elastic:master Jul 22, 2020
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Jul 22, 2020
Merged
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Jul 22, 2020
@alvarezmelissa87
[ML] API integration tests for UPDATE data frame analytics endpoint (e…
66354a1 
…lastic#72710)

* add df analytics update api integration tests

* remove unnecessary commented code

* remove unused constant

* fetch job to check it was updated correctly
@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfanalytics-update-tests branch July 22, 2020 20:52
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 23, 2020
@gmmorris
Merge branch 'master' into task-manager/fix-flaky-timers
b4e78e7 
* master: (35 commits)
  Migrated karma tests to jest (elastic#72649)
  Migrate status page app to core (elastic#72017)
  Failing test: Jest Tests.src/plugins/vis_type_vega/public (elastic#71834)
  Fix Firefox TSVB flaky test with switch index patterns (elastic#72882)
  [ML] Fixing link to index management from file data visualizer (elastic#72863)
  test: 💍 add test for sub-expression variables (elastic#71644)
  fix bug (elastic#72809)
  [keystore] use get_keystore in server cli (elastic#72954)
  Show step number instead of incomplete step. (elastic#72866)
  Fix bug where user can't add an exception when "close alert" is checked (elastic#72919)
  [Monitoring] Fix issues displaying alerts (elastic#72891)
  [Ingest Manager] Add more Fleet concurrency tests elastic#71744 (elastic#72338)
  [Security Solution][Exceptions] - Update UI exceptions builder nested logic (elastic#72490)
  disable renovate masterIssue
  [ML] API integration tests for UPDATE data frame analytics endpoint (elastic#72710)
  [Uptime] Fix accessibility issue in Uptime app nav links (elastic#72926)
  [Maps] fix removing global filter from layer can cause app to start thrashing (elastic#72763)
  [Maps] fix blended layer aggregation error when using composite aggregation (elastic#72759)
  fix unexpected arguments to unload command
  Limits the upload size of lists to 9 meg size (elastic#72898)
  ...
alvarezmelissa87 added a commit that referenced this pull request Jul 23, 2020
@alvarezmelissa87
[ML] API integration tests for UPDATE data frame analytics endpoint (#…
16a54be 
…72710) (#72966)

* add df analytics update api integration tests

* remove unnecessary commented code

* remove unused constant

* fetch job to check it was updated correctly
@peteharverson peteharverson mentioned this pull request Feb 25, 2021
Closed
90 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qn895 qn895 qn895 left review comments

@peteharverson peteharverson peteharverson approved these changes

@pheyos pheyos pheyos approved these changes

Assignees

@alvarezmelissa87 alvarezmelissa87

Labels
Feature:Data Frame Analytics ML data frame analytics features :ml release_note:skip Skip the PR/issue when compiling release notes test-api-integration v7.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@alvarezmelissa87 @elasticmachine @qn895 @pheyos @kibanamachine @peteharverson