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

Removed incorrect test #39400

Merged
merged 1 commit into from
Mar 5, 2019
Merged

Conversation

delvedor
Copy link
Member

There is no need for this test since the clients will only validate parameters, and not logic/state (eg, a client cannot know that the job_id is already taken).

cc @elastic/es-clients

@delvedor delvedor requested a review from droberts195 February 26, 2019 14:19
@droberts195 droberts195 added >test Issues or PRs that are addressing/adding tests :ml Machine learning v8.0.0 v7.2.0 labels Feb 26, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@droberts195
Copy link
Contributor

droberts195 commented Feb 26, 2019

For the benefit of anyone else who looks at this and wonders why that test isn't changed to assert an error response from the server, the answer is that there is a test that does exactly that immediately above the deleted one. I think the test that asserted that a client-side error would be triggered must have been a cut-and-paste error at the time the test file was first created. And when running in the ES build system the framework ignores the results of tests that are supposed to trigger a client-side error, which is why it doesn't cause ES builds to fail. It only causes problems for the client testing.

@droberts195 droberts195 merged commit 80a109b into elastic:master Mar 5, 2019
droberts195 pushed a commit that referenced this pull request Mar 5, 2019
A client cannot know that a job_id is already taken, so
this test should not have been specified as a client test
droberts195 pushed a commit that referenced this pull request Mar 5, 2019
A client cannot know that a job_id is already taken, so
this test should not have been specified as a client test
droberts195 pushed a commit that referenced this pull request Mar 5, 2019
A client cannot know that a job_id is already taken, so
this test should not have been specified as a client test
droberts195 pushed a commit that referenced this pull request Mar 5, 2019
A client cannot know that a job_id is already taken, so
this test should not have been specified as a client test
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 6, 2019
* 6.7: (39 commits)
  Remove beta label from CCR (elastic#39722)
  Rename retention lease setting (elastic#39719)
  Add Docker build type (elastic#39378)
  Use any index specified by .watches for Watcher (elastic#39541) (elastic#39706)
  Add documentation on remote recovery (elastic#39483)
  fix typo in synonym graph filter docs
  Removed incorrect ML YAML tests (elastic#39400)
  Improved Terms Aggregation documentation (elastic#38892)
  Fix Fuzziness#asDistance(String) (elastic#39643)
  Revert "unmute EvilLoggerTests#testDeprecatedSettings (elastic#38743)"
  Mute TokenAuthIntegTests.testExpiredTokensDeletedAfterExpiration (elastic#39690)
  Fix security index auto-create and state recovery race (elastic#39582)
  [DOCS] Sorts security APIs
  Check for .watches that wasn't upgraded properly (elastic#39609)
  Assert recovery done in testDoNotWaitForPendingSeqNo (elastic#39595)
  [DOCS] Updates API in Watcher transform context (elastic#39540)
  Fixing the custom object serialization bug in diffable utils. (elastic#39544)
  mute test
  SQL: Don't allow inexact fields for MIN/MAX (elastic#39563)
  Update release notes for 6.7.0
  ...
@delvedor delvedor deleted the fix-ml-yaml-test branch April 14, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml Machine learning >test Issues or PRs that are addressing/adding tests v6.6.2 v6.7.0 v7.0.0-rc1 v7.2.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants