elastic / elasticsearch

Free and Open, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
68.55k stars 24.35k forks source link

Missing rest specs for testing features' code snippets #48663

Open lcawl opened 4 years ago

lcawl commented 4 years ago

The docs/build.gradle that we use for code snippet testing doesn't seem to parse all the X-Pack API specs. For example, in https://github.com/elastic/elasticsearch/pull/44018 we received:

org.elasticsearch.smoketest.DocsClientYamlTestSuiteIT > test {yaml=reference/ml/apis/delete-dfanalytics/line_38} FAILED 08:43:32 java.lang.RuntimeException: Failure at [reference/ml/apis/delete-dfanalytics:33]: rest api [ml.put_data_frame_analytics] doesn't exist in the rest spec

We have worked around this for rollups and for a handful of ML APIs by using "raw" methods in the build.gradle file. For example: https://github.com/elastic/elasticsearch/pull/48259

Instead of implementing these work-arounds, however, it would be preferable to have all the rest specs recognized by the documentation tests.

elasticmachine commented 4 years ago

Pinging @elastic/es-core-infra (:Core/Infra/Build)

lcawl commented 4 years ago

Per @droberts195:

We have cleanup code both for the X-Pack REST tests [1] and the (Apache 2 license) HLRC tests [2].

The docs tests in the x-pack/docs directory already use the X-Pack ML REST test cleaner, because XDocsClientYamlTestSuiteIT extends XPackRestIT [3] and that uses the MlRestTestStateCleaner.

The only ML cleanup for the main docs directory tests is to wait for pending tasks [4]. So ironically the x-pack/docs tests are doing ML cleanup after each test (don’t worry - it should be very fast when there are no ML jobs) whereas the tests of the directory where the ML documentation actually lives are not.

One way we could solve both the cleanup problem and the spec problem would be to move all the ML docs into the x-pack/docs directory...The other way would be to either duplicate the HLRC ML cleanup class into the docs test code or put it somewhere where both sets of tests can use it...

[1] https://github.com/elastic/elasticsearch/blob/master/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/integration/MlRestTestStateCleaner.java#L19 [2] https://github.com/elastic/elasticsearch/blob/master/client/rest-high-level/src/test/java/org/elasticsearch/client/MlTestStateCleaner.java#L42 [3] https://github.com/elastic/elasticsearch/blob/master/x-pack/docs/src/test/java/org/elasticsearch/smoketest/XDocsClientYamlTestSuiteIT.java#L32 [4] https://github.com/elastic/elasticsearch/blob/master/docs/src/test/java/org/elasticsearch/smoketest/DocsClientYamlTestSuiteIT.java#L103

elasticmachine commented 4 years ago

Pinging @elastic/es-docs (:Docs)

mark-vieira commented 3 years ago

@lcawl Does this continue to be a problem? I think with the change to remove the OSS distribution this simplifies things a bit WRT to testing as we no longer will have the notion of "OSS" docs.

@jakelandis on a related point, I wonder to what degree we can simplify the distinction between "oss" and "x-pack" for REST API specs in generally, given that the default distribution is now the only one that will exist :thinking:

lcawl commented 3 years ago

I haven't tried removing them lately, but I still see "raw" methods in the build.gradle, such as https://github.com/elastic/elasticsearch/blob/974c9aaf40f29bad12351a94f6995340d03ec073/docs/build.gradle#L762

jakelandis commented 3 years ago

I wonder to what degree we can simplify the distinction between "oss" and "x-pack" for REST API specs in generally, given that the default distribution is now the only one that will exist

@mark-vieira - It would be great to simply move the x-pack specs (x-pack/plugin/src/test/resources/rest-api-spec/api/**) to same directory as the non-x-pack specs (rest-api-spec/src/main/resources/rest-api-spec/api). To allow things to keep working it should be as easy find all the configurations like this:

restResources {
  restApi {
    includeCore '_common', 'cluster', 'nodes', 'indices', 'index', 'info'
    includeXpack 'ccr'
  }
}

and change it to

restResources {
  restApi {
    include '_common', 'cluster', 'nodes', 'indices', 'index', 'info', `ccr`
  }
}

There are probably a couple edge cases...but that would be the primary change for ES. The restResources are essentially 2 buckets of files, and this would allow us to simply remove one of the buckets (the test buckets would remain unchanged). This would likely break the client's team since they rely on the the directory structure. Additionally the artifact that we publish would now include the x-pack specs, so not sure if there are licensing concerns there.

We also support project specific API's (specs), but I think there is only 1 project (lang-painless) that actually does that.

Instead of implementing these work-arounds, however, it would be preferable to have all the rest specs recognized by the documentation tests.

@lcawl - I think you should be able to change the configuration from:

restResources {
  restApi {
    includeCore '*'
  }
}

to

restResources {
  restApi {
    includeCore '*'
    includeXpack '*'
  }
}

To bring in all the x-pack specs into docs/build.gradle. This would only put the specs on the classpath, but it doesn't influence the test setup/cleanup.

mark-vieira commented 3 years ago

@jakelandis that's pretty much what I was thinking.

elasticsearchmachine commented 1 year ago

Pinging @elastic/es-delivery (Team:Delivery)