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

Improve Elasticsearch integration tests #1563

Conversation

nr-ahemsath
Copy link
Member

Description

Improves Elasticsearch integration tests by:

  • Adding tests for the following operations: IndexMany, IndexManyAsync, MultiSearch, MultiSearchAsync
  • Creating a single ValidateOperation method that takes the operation name as an argument, and performs assertions accordingly. This replaces multiple nearly identical validation methods
  • Combining the sync and async tests into a single test class
  • Increasing the range of client versions being tested
  • Reducing the overall number of test app executions by only testing the oldest and newest framework and core versions

The exerciser code still isn't validating that its operations are actually succeeding. I started down this path for a bit and ran into a number of issues, so I want to get what I've done PR'ed before continuing with that work.

Note: for the Elastic.Clients.Elasticsearch (8.x) case, the code for MultiSearch(Async) isn't actually working. I was unable to find documentation on how to use this API in 8.x, nor could I find any examples online, and translating working 7.x examples to 8.x did not work. The empty invocation of the API is still enough to trigger the instrumentation wrapper, and the test assertions succeed accordingly.

Author Checklist

  • Unit tests, Integration tests, and Unbounded tests completed
  • Performance testing completed with satisfactory results (if required)

Reviewer Checklist

  • Perform code review
  • Pull request was adequately tested (new/existing tests, performance tests)

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (feature/elasticsearch-instrumentation@ed735bd). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                           Coverage Diff                            @@
##             feature/elasticsearch-instrumentation    #1563   +/-   ##
========================================================================
  Coverage                                         ?   72.99%           
========================================================================
  Files                                            ?      408           
  Lines                                            ?    25604           
  Branches                                         ?        0           
========================================================================
  Hits                                             ?    18690           
  Misses                                           ?     6914           
  Partials                                         ?        0           

Copy link
Member

@chynesNR chynesNR left a comment

Choose a reason for hiding this comment

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

Looks good!

@nr-ahemsath nr-ahemsath merged commit 69737f3 into feature/elasticsearch-instrumentation Apr 20, 2023
@nr-ahemsath nr-ahemsath deleted the nr-ahemsath/expand-elasticsearch-integration-tests branch April 24, 2023 17:28
nr-ahemsath added a commit that referenced this pull request Apr 26, 2023
* Port initial poc

* Added elastic search and kibana to unbounded services (#1505)

* Enabled authentication for ElasticSearch and Kibana (#1507)

* Updated docker-compose to enable authentication for ElasticSearch and Kibana. 
* Added a configuration helper for use in integration tests for ElasticSearch.

* Support Elastic.Clients.Elasticsearch (8.x) client (#1512)

* Add new instrumentation files to build artifacts (#1529)

* test: Elasticsearch integration tests framework (#1532)

* Instrument async Elasticsearch methods (#1535)

* Working async instrumentation

* Get NEST async working, fix integration test

* Set URI after datastore segment creation (#1538)

* Initial plumbing, seems to work

* Update integration tests

* Remove stray using

* Clean up comments

* Build bug and review feedback

* Handle https and improve async reliability

Handle https as well as http in Elastic server url
Fix async timing problem with a delay (ugh)

* Incremental Elasticsearch integration test work (#1543)

* Report Elasticsearch.net operations more accurately (#1559)

* Improve Elasticsearch integration tests (#1563)

* Fix solution build problem

* Add search validation to async tests

Also remove comments from wrapper
Also reorder methods in sync tests

* Initial coding of bulk insert and multisearch in exercisers

* Added sync tests for indexmany and multisearch.  Not passing.

* Fix assertions

* Add async operations to sync tests class

* Combine sync and async test files

* Get elastic.clients case to work

* Elasticsearch.Net tests passing

* Test range of versions but only test oldest and newest frameworks

* Add client call success validation to Elasticsearch integration tests (#1566)

* Bubble up the async await

* Add API operation validation

All tests passing locally

* Elasticsearch error reporting (#1568)

* Adding support for Elasticsearch error reporting

* Safer way to set errors

* Fixed unreliable integration tests; fixed potential path parsing crash

* Missed replacing a call to InternalApi

---------

Co-authored-by: Marty Tippin <120425148+tippmar-nr@users.noreply.github.com>
Co-authored-by: Chris Hynes <111462425+chynesNR@users.noreply.github.com>
Co-authored-by: chynesNR <chynes@newrelic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants