Introduce REST test transformations #67268

jakelandis · 2021-01-11T17:13:51Z

This commit introduces the ability to programmatically transform YAML based REST tests. Specifically this initial commit introduces the ability to inject HTTP headers into the YAML REST tests. Additional capabilities (transforms) will be introduced in future commits.

Transforming REST tests is a key component to test REST API compatibility. Eventually (not included here) the ability to transform tests will be integrated with Gradle such that the tests that are transformed prior to being executed. This commit does not have any REST API compatibility specific changes, and only introduces the code that will be used for that testing.

Transforming REST tests at the YAML level was chosen over updating the test runners to ensure that these tests may (at a future time) be used by the various clients that re-use the YAML based REST tests. By transforming the tests themselves, there is a consistent view across all runners for exactly how and what to test when running N-1 tests against a N cluster with REST compatibility.

To get an idea of how this will be used:
https://github.com/jakelandis/elasticsearch/blob/compat_header_inject_with_gradle/modules/ingest-common/build.gradle#L20
https://github.com/jakelandis/elasticsearch/blob/compat_header_inject_with_gradle/buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/YamlRestCompatTestPlugin.java#L148
https://github.com/jakelandis/elasticsearch/blob/compat_header_inject_with_gradle/buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/RestCompatTestTransformTask.java

I also added "humanDebug" flag to the tests that will output the transformed tests to stdout in case you want to run the tests and see the output directly.

jakelandis · 2021-01-12T14:18:11Z

@elasticmachine update branch

elasticmachine · 2021-01-12T16:35:09Z

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

nik9000 · 2021-01-12T17:56:14Z

This seems like a very reasonable way to do it to me. I'd love to be able to cat the transformed yml files and this seems like a very reasonable way to get that.

nik9000

Given that @mark-vieira is going to be pretty busy I think we can get this in now and beg forgiveness later if we find something untoward. It makes sense to me. We'll get a closer review on the actual build changes required when we build the files and such.

mark-vieira

I left a few comments, otherwise I'll defer to someone that's more familiar with the YAML test stuff to review.

We'll definitely want to chat about how to do the build integration stuff. Effectively, the transforms themselves will be an input to these tasks so we need to track those implementations as inputs, which can be tricky depending on where those implementations live.

mark-vieira · 2021-01-14T20:16:22Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/transform/RestTestTransformer.java

+     * @param currentNode      The current node that is being evaluated.
+     * @param objectKeyFinders A Map of the key to find to the
+     */
+    private void transformByObjectKeyName(JsonNode currentNode, Map<String, RestTestTransformByObjectKey> objectKeyFinders) {


This seems like something that should exist in the transform, as again, we are now coupled to the corresponding interface via an instanceof check. Perhaps put this in an abstract implementation of RestTestTransform transformTest method and have InjectHeaders extend that.

The RestTestTransformer operates against the whole YAML test, and the implementations of RestTestTransform operate only on the specific node passed (from one or multiple tests) that it passed. This is to help minimize the number of times we need to traverse the JSON tree. By keeping the JSON tree traversal here (as opposed to the individual transform) allows for multiple transforms with a single traversal for all transforms that are initiated by a named object key. Currently it is only looks for "do" to inject headers, but future transforms will will be able to implement that interface with a different object key name and do something different like replace the value of object. Further, this method will likely get extended and renamed in future iterations to allow for more traits that kick off a transform (i.e. replacing one text value with another) while keeping the JSON traversals needed at a minimum.

Understood. Does JsonNode not contain a reference to it's parent such that the transform itself could make the deterinmation it's "inside" a do: block?

It does not maintain a parent reference, however, with the recursive approach here it is pretty simple to carry forward the parent (which is only ever an object or an array which kicks off the recursion).

Not sure that would help however, the goal here is to find all objects with the key "do" once we find that then we can transform it. In this specific case we only need the object itself, in other (future) cases we will need to pass the parent not the thing found. For example if we need an AddSyblingObject then we would need to pass in the parent of found object.

Perhaps I could change the name of the method to avoid confusion... the method should probably be called traverseTest.

mark-vieira · 2021-01-14T20:18:46Z

buildSrc/src/test/java/org/elasticsearch/gradle/test/rest/transform/InjectHeaderTests.java

+import java.util.concurrent.atomic.LongAdder;
+import java.util.stream.Collectors;
+
+public class InjectHeaderTests extends GradleUnitTestCase {


Boy, I wish we could start moving some of these unit tests to spock as well but that's outside the scope of this PR.

mark-vieira · 2021-01-14T20:25:30Z

@breskeby heads up on this as well. This is very reminiscent of what I did for the Gradle documentation stuff 😉. I remember making cacheability work properly on that was a non-trivial effort and we'll want to ensure the same thing here.

pgomulka

I left two minor comments
LGTM

pgomulka · 2021-01-15T09:17:42Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/transform/RestTestTransformer.java

+                if ("teardown".equals(testName)) {
+                    teardownSection = test;
+                }
+                Map<String, RestTestTransformByObjectKey> objectKeyFinders = transformations.stream()


this could be done before the tests for loop?

indeed it can, nice catch.

pgomulka · 2021-01-15T09:27:05Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/transform/RestTestTransformer.java

+            int i = 0;
+            for (RestTestTransformGlobalSetup setupTransform : setupTransforms) {
+                ObjectNode result = setupTransform.transformSetup(setupSection);
+                if (result != null && setupSection == null && i++ == 0) {


can you put a comment about this expression?
Is the intention that if there is no setupSection, but there is a transformation for a setup, then add a setup?
but it also looks like only one can be added? What if I have two setup transformations ?

can you put a comment about this expression?

will do.

Is the intention that if there is no setupSection, but there is a transformation for a setup, then add a setup?

yes, that is the intention (create one if missing)

but it also looks like only one can be added? What if I have two setup transformations ?

good catch . There is a bug as-is with multiple setup/teardown transforms which will get fixed ... however since there is only at most 1 with this PR it is not tested yet , but will likely run into that sooner then later once i introduce more transforms.

edit: changed method name 8af24f0

jakelandis · 2021-01-15T17:41:05Z

@elasticmachine update branch

jakelandis · 2021-01-19T15:46:53Z

Thanks everyone for the reviews !

This commit introduces the ability to programmatically transform YAML based REST tests. Specifically this initial commit introduces the ability to inject HTTP headers into the YAML REST tests. Additional capabilities (transforms) will be introduced in future commits. Transforming REST tests is a key component to test REST API compatibility. Eventually (not included here) the ability to transform tests will be integrated with Gradle such that the tests that are transformed prior to being executed. This commit does not have any REST API compatibility specific changes, and only introduces the code that will be used for that testing. Transforming REST tests at the YAML level was chosen over updating the test runners to ensure that these tests may (at a future time) be used by the various clients that re-use the YAML based REST tests. By transforming the tests themselves, there is a consistent view across all runners for exactly how and what to test when running N-1 tests against a N cluster with REST compatibility.

jakelandis added 6 commits January 6, 2021 15:16

initial commit

2382ab2

implement inject header

967b4f4

support setup

d1146f2

finish up initial test for inject header

1cdbafb

more tests

453119d

precommit

dbfdac5

jakelandis added the WIP label Jan 11, 2021

jakelandis added 4 commits January 11, 2021 12:12

minor cleanup

ed55696

Merge remote-tracking branch 'upstream/master' into compat_header_inject

abf3444

updates (from other branch)

74b9bc9

minor clean up

89dd856

elasticmachine and others added 2 commits January 12, 2021 06:18

Merge branch 'master' into compat_header_inject

4de9371

add manual debug option

e2d0b0e

jakelandis changed the title ~~WIP~~ Introduce REST test transformations Jan 12, 2021

jakelandis added :Delivery/Build Build or test infrastructure >enhancement v8.0.0 >non-issue and removed WIP labels Jan 12, 2021

jakelandis requested review from mark-vieira, pgomulka and nik9000 January 12, 2021 16:26

jakelandis marked this pull request as ready for review January 12, 2021 16:35

elasticmachine added the Team:Delivery Meta label for Delivery team label Jan 12, 2021

nik9000 approved these changes Jan 14, 2021

View reviewed changes

mark-vieira approved these changes Jan 14, 2021

View reviewed changes

pgomulka approved these changes Jan 15, 2021

View reviewed changes

move collection and fix multi setup/teardown

036d705

Merge branch 'master' into compat_header_inject

f1242c0

jakelandis mentioned this pull request Jan 15, 2021

Move yamlRestCompatTest plugin to internal and add a new lifecycle #67394

Merged

jakelandis added 2 commits January 15, 2021 16:39

Merge remote-tracking branch 'upstream/master' into compat_header_inject

68b4bd2

minor: rename method

8af24f0

jakelandis added the v7.12.0 label Jan 19, 2021

jakelandis merged commit e3dc69b into elastic:master Jan 19, 2021

jakelandis deleted the compat_header_inject branch January 19, 2021 15:48

jakelandis added the backport pending label Jan 19, 2021

jakelandis mentioned this pull request Jan 19, 2021

[7.x] Introduce REST test transformations (#67268) #67715

Merged

jakelandis removed the backport pending label Jan 21, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce REST test transformations #67268

Introduce REST test transformations #67268

jakelandis commented Jan 11, 2021 •

edited

Loading

jakelandis commented Jan 12, 2021

elasticmachine commented Jan 12, 2021

nik9000 commented Jan 12, 2021

nik9000 left a comment

mark-vieira left a comment

mark-vieira Jan 14, 2021

jakelandis Jan 15, 2021

mark-vieira Jan 15, 2021

jakelandis Jan 15, 2021

mark-vieira Jan 14, 2021

mark-vieira commented Jan 14, 2021

pgomulka left a comment

pgomulka Jan 15, 2021

jakelandis Jan 15, 2021

pgomulka Jan 15, 2021

jakelandis Jan 15, 2021 •

edited

Loading

jakelandis commented Jan 15, 2021

jakelandis commented Jan 19, 2021

Introduce REST test transformations #67268

Introduce REST test transformations #67268

Conversation

jakelandis commented Jan 11, 2021 • edited Loading

jakelandis commented Jan 12, 2021

elasticmachine commented Jan 12, 2021

nik9000 commented Jan 12, 2021

nik9000 left a comment

Choose a reason for hiding this comment

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira commented Jan 14, 2021

pgomulka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

jakelandis commented Jan 15, 2021

jakelandis commented Jan 19, 2021

jakelandis commented Jan 11, 2021 •

edited

Loading

jakelandis Jan 15, 2021 •

edited

Loading