-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Rest API Spec tests runner #4123
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
Conversation
russcam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one serious PR! 🎉
I've had an initial pass-through and left some comments. I'd like to dig more thoroughly into the F# project
This commit adds Powershell script equivalents of the ci shell scripts, to allow yaml tests to run on Windows with Docker
russcam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some more comments. I think it would be good to have some unit tests for the F# project
src/Elasticsearch.Net/Serialization/Formatters/DynamicDictionaryFormatter.cs
Show resolved
Hide resolved
| @@ -0,0 +1,221 @@ | |||
| module Tests.YamlRunner.Models | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some XML comments (///) on types would help in navigating what each is used for. The majority of them look to directly map to YAML test runner concepts, are there URLs that can be linked to, that have definitions for each too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F# editor support is pretty great but this is a bit of a weak spot, rider offers no help and Ionide generates DocFx comments.
Okay to follow up in a new PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
| open Tests.YamlRunner.Models | ||
| open Elasticsearch.Net | ||
|
|
||
| type ApiInvoke = delegate of Object * Object[] -> Task<DynamicResponse> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some XML comments to indicate what each type is used for?
|
I've added some PowerShell scripts to allow running the CI flow on Windows. I know that CI isn't going to run these, but it's useful to be able to just run these scripts locally to test |
|
Thanks for the notes and PS script @russcam 👍 |
No worries! With Docker for Windows, will need to give permission to docker to be able to create files in the repo directory and descendents; may want to create a separate Windows account for this purpose, for docker to use. |
…ests and prints the PID
Co-Authored-By: Russ Cam <russ.cam@elastic.co>
Co-Authored-By: Russ Cam <russ.cam@elastic.co>
Co-Authored-By: Russ Cam <russ.cam@elastic.co>
…elasticsearch-net into feature/master/yaml-test-runner
|
@russcam thanks for the review, other than the xmldocs addresses your points. Can you give this the 👍 or 👎 , looking to get this in to iterate over in smaller PR's |
|
This is looking good, @Mpdreamz 👍 |
* stage, now actually sends querystring parameters * DynamicResponse now useful in the context of yaml runner * support _arbitrary_key_ * more assertions * more assertions * more assertions * gh workflow, some changes to allow client url to be passed on commandlie * update workflow * update docker image ref * update docker image ref * update health checks * attach for visibility * --no-healthcheck not valid with -a * no health check * add limits * explicit hostname * single node discovery * minimize options * wrong .NET sdk * dotnet-run should have been dotnet run * need -- to separate args * actions/setup-dotnet#29 :sadpanda: * print some info to assert container information * cleaned up program a bit now discovers revision to download from running elasticsearch version * junit xml exporter * can now pass in output file as arg, report totals on stdout * export to artifacts * yaml * yaml spacing * make sure build/output exists * always upload artifact * test out logging commands * update es container config * reenable ES_JAVA_OPTS * java opts not able to set to 1g enable memory_lock * drop single-node discovery-type * back to single node * stage * .jenkins build * update our .jenkins folder * update runner to pretty print failures * move from .jenkins folder to .ci * add readme to the .ci folder * attempt to show logs in github actions * disable github actions for now * regenerate code after rebase * unit test for __arbitrary_key__ used the wrong meaning * Add PowerShell scripts to run on Windows This commit adds Powershell script equivalents of the ci shell scripts, to allow yaml tests to run on Windows with Docker * added --profile switch to yaml runner that waits before running the tests and prints the PID * update ci jobs go-elasticsearch references * Update src/Elasticsearch.Net/Responses/Dynamic/DynamicDictionary.cs Co-Authored-By: Russ Cam <russ.cam@elastic.co> * only automatically set proxy on client if we see mitmproxy running * Update src/Tests/Tests.YamlRunner/DoMapper.fs Co-Authored-By: Russ Cam <russ.cam@elastic.co> * Update .ci/run-tests Co-Authored-By: Russ Cam <russ.cam@elastic.co> * Addressed various PR feedback (cherry picked from commit c3f85a0)
* Rest API Spec tests runner (#4123) * stage, now actually sends querystring parameters * DynamicResponse now useful in the context of yaml runner * support _arbitrary_key_ * more assertions * more assertions * more assertions * gh workflow, some changes to allow client url to be passed on commandlie * update workflow * update docker image ref * update docker image ref * update health checks * attach for visibility * --no-healthcheck not valid with -a * no health check * add limits * explicit hostname * single node discovery * minimize options * wrong .NET sdk * dotnet-run should have been dotnet run * need -- to separate args * actions/setup-dotnet#29 :sadpanda: * print some info to assert container information * cleaned up program a bit now discovers revision to download from running elasticsearch version * junit xml exporter * can now pass in output file as arg, report totals on stdout * export to artifacts * yaml * yaml spacing * make sure build/output exists * always upload artifact * test out logging commands * update es container config * reenable ES_JAVA_OPTS * java opts not able to set to 1g enable memory_lock * drop single-node discovery-type * back to single node * stage * .jenkins build * update our .jenkins folder * update runner to pretty print failures * move from .jenkins folder to .ci * add readme to the .ci folder * attempt to show logs in github actions * disable github actions for now * regenerate code after rebase * unit test for __arbitrary_key__ used the wrong meaning * Add PowerShell scripts to run on Windows This commit adds Powershell script equivalents of the ci shell scripts, to allow yaml tests to run on Windows with Docker * added --profile switch to yaml runner that waits before running the tests and prints the PID * update ci jobs go-elasticsearch references * Update src/Elasticsearch.Net/Responses/Dynamic/DynamicDictionary.cs Co-Authored-By: Russ Cam <russ.cam@elastic.co> * only automatically set proxy on client if we see mitmproxy running * Update src/Tests/Tests.YamlRunner/DoMapper.fs Co-Authored-By: Russ Cam <russ.cam@elastic.co> * Update .ci/run-tests Co-Authored-By: Russ Cam <russ.cam@elastic.co> * Addressed various PR feedback (cherry picked from commit c3f85a0) * move to 7. snapshot * clean up sln * DynamicDictionary Get should deal with nullables * update jenkins build scripts to latest * Add support for Get<DynamicDictionary>() on DynamicDictionary * Add more debug assertions around flakey test
* Rest API Spec tests runner (#4123) * stage, now actually sends querystring parameters * DynamicResponse now useful in the context of yaml runner * support _arbitrary_key_ * more assertions * more assertions * more assertions * gh workflow, some changes to allow client url to be passed on commandlie * update workflow * update docker image ref * update docker image ref * update health checks * attach for visibility * --no-healthcheck not valid with -a * no health check * add limits * explicit hostname * single node discovery * minimize options * wrong .NET sdk * dotnet-run should have been dotnet run * need -- to separate args * actions/setup-dotnet#29 :sadpanda: * print some info to assert container information * cleaned up program a bit now discovers revision to download from running elasticsearch version * junit xml exporter * can now pass in output file as arg, report totals on stdout * export to artifacts * yaml * yaml spacing * make sure build/output exists * always upload artifact * test out logging commands * update es container config * reenable ES_JAVA_OPTS * java opts not able to set to 1g enable memory_lock * drop single-node discovery-type * back to single node * stage * .jenkins build * update our .jenkins folder * update runner to pretty print failures * move from .jenkins folder to .ci * add readme to the .ci folder * attempt to show logs in github actions * disable github actions for now * regenerate code after rebase * unit test for __arbitrary_key__ used the wrong meaning * Add PowerShell scripts to run on Windows This commit adds Powershell script equivalents of the ci shell scripts, to allow yaml tests to run on Windows with Docker * added --profile switch to yaml runner that waits before running the tests and prints the PID * update ci jobs go-elasticsearch references * Update src/Elasticsearch.Net/Responses/Dynamic/DynamicDictionary.cs Co-Authored-By: Russ Cam <russ.cam@elastic.co> * only automatically set proxy on client if we see mitmproxy running * Update src/Tests/Tests.YamlRunner/DoMapper.fs Co-Authored-By: Russ Cam <russ.cam@elastic.co> * Update .ci/run-tests Co-Authored-By: Russ Cam <russ.cam@elastic.co> * Addressed various PR feedback (cherry picked from commit c3f85a0) * move to 7. snapshot * clean up sln * DynamicDictionary Get should deal with nullables * update jenkins build scripts to latest * Add support for Get<DynamicDictionary>() on DynamicDictionary * Add more debug assertions around flakey test (cherry picked from commit 655b5ce)
Opening this PR since this feature branch is becoming too big to manage.
what
This re-introduces the ability for us to run the rest api spec tests
Which are defined in yaml as a sequence of steps to run in the elasticsearch codebase itself.
Why
Prior to
2.0we supported this by transforming the yaml into xunit csharp code that we checked into git. We decided to remove this as we had/have an extensive unit/integration test suite already and these tests became more an excersise in how well can we translate yaml into c# code (or more specifically anonymous C# classes).We are reintroducing this for multiple reasons:
NEST.NESTusesElasticsearch.Netbut this gives us an opportunity to testElasticsearch.Netand the more dynamic/freeform use cases.How
This time around we no longer translate the yaml test files to xunit C# project that we then compile and run.
Instead we created a runner that translates the yaml at run time and dispatches calls into an
IElasticsearchClientinstance.In order to help reflection when we generate
IElasticsearchClientit also injects private attributes on the method e.g[MapsApi("namespace.method", "path1,path2")]this takes the guessing out of the equation which method to call.The runner itself lives under
src/Tests/Tests.YamlRunneras anF#program.It's
mainis hopefully self documents the tests we take to run these testsInvoke
The runner builds with the rest of the projects and is not special in that sense.It's invocation is a little bit special though by design. We try hard to fully automate all our procedures through
build/scripts/scripts.fsprojon any OS.The runner however expects the elasticsearch endpoint to be spun up before invocation. To do this all the clients standardize on the same script (
.ci/run-elasticsearch.sh) which only exists as a shell script. This ties this part to aPOSIXshell.The actual invocation of the runner however can still be done through our
.build.sh rest-spec-testor.build.bat rest-spec-testThe
readme.mdundercigoes further into the setup.if you have a POSIX shell (that can start bash) and docker installed locally you can call
ELASTICSEARCH_VERSION=8.0.0-SNAPSHOT ..ci/run-testswhich is exactly what Jenkins over at https://clients-ci.elastic.co will end up calling.
The
.cifolder is mostly ready to be setup internally with our infra team, it makes sense for this to land inmasterbefore we submit a PR internally to get it up though.This PR also contains a github actions workflow that i tinkered with but will leave disabled for now. It has a lot of potential to standardize our common actions steps across clients but lacks reporting tools for now. Interacting with Github Checks is hard and there is no tests visualizer just yet.
On Azure Devops you can log special messages that appear as github checks. We use this on our normal integration tests to surface reproduce commands without having to dig through logs for instance.
Given the close relation of Azure Devops and Github Actions I hope these are only a matter of time.
Whats next
Not all the tests actually pass sadly, this is in part because we don't have a block list for certain files in the same way other runners do and in part because some features are not yet implemented (transform_and_set / headers come to mind).
For now I treat these failures as a feature since the runner also exports an
junit.xmlfile that we create manually. I would like to see this file visualized in Jenkins with failures to know my xml juggling produced something Jenkins understands.Will follow up with a new meta ticket detailing what's outstanding post merge.