Feature/7.x/virtual connection #3908
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mpdreamz
force-pushed
the
feature/7.x/virtual-connection
branch
from
4fb2b02 to
f3f5280
Compare
Member
Author
|
Rebased, the idea behind spinning this off into its own project is To help testing, since many interfaces are gone mocking is not an option. This is by design we feel that you should mock the boundaries you control in your application (service layers/etc) injecting a connection that behaves a certain way can aid with testing though and E.g for elastic/apm-agent-dotnet#329 I want to use this to write unit test for the clients integration with APM. |
Mpdreamz
force-pushed
the
feature/7.x/virtual-connection
branch
from
f3f5280 to
9038208
Compare
russcam
reviewed
Aug 27, 2019
Contributor
russcam
left a comment
russcam
left a comment
There was a problem hiding this comment.
The changes look good, but I left some comments around naming and descriptions
|
|
||
| namespace Elasticsearch.Net.Virtual | ||
| { | ||
| public class VirtualizedCluster |
Contributor
There was a problem hiding this comment.
VirtualCluster, to align with VirtualConnection?
| <?xml version="1.0" encoding="utf-8"?> | ||
| <package xmlns="http://schemas.microsoft.com/packaging/2012/06/nuspec.xsd"> | ||
| <metadata> | ||
| <id>Elasticsearch.Net.Virtual</id> |
Contributor
There was a problem hiding this comment.
Naming is hard 😃
Elasticsearch.Net.Virtual feels somewhat vague. The package is a way to mock/stub a cluster, with a specific IConnection implementation. Would Elasticsearch.Net.VirtualCluster, or Elasticsearch.Net.MockCluster or Elasticsearch.Net.StubCluster or Elasticsearch.Net.FakeCluster better convey the usage?
| <iconUrl>https://raw.githubusercontent.com/elastic/elasticsearch-net/master/build/nuget-icon.png</iconUrl> | ||
| <requireLicenseAcceptance>false</requireLicenseAcceptance> | ||
| <summary>Provides a way to assert cluent behaviour through a rule engine backed VirtualClusterConnection</summary> | ||
| <description>Provides a way to assert cluent behaviour through a rule engine backed VirtualClusterConnection</description> |
Contributor
There was a problem hiding this comment.
Suggested change
| <description>Provides a way to assert cluent behaviour through a rule engine backed VirtualClusterConnection</description> | |
| <description>Provides a way to assert client behaviour through a rule engine backed VirtualClusterConnection</description> |
| <summary>Provides a way to assert cluent behaviour through a rule engine backed VirtualClusterConnection</summary> | ||
| <description>Provides a way to assert cluent behaviour through a rule engine backed VirtualClusterConnection</description> | ||
| <releaseNotes>See https://github.com/elastic/elasticsearch-net/releases/tag/$version$</releaseNotes> | ||
| <copyright>2014-$year$ Elasticsearch BV</copyright> |
Contributor
There was a problem hiding this comment.
Suggested change
| <copyright>2014-$year$ Elasticsearch BV</copyright> | |
| <copyright>2019-$year$ Elasticsearch BV</copyright> |
| <description>Provides a way to assert cluent behaviour through a rule engine backed VirtualClusterConnection</description> | ||
| <releaseNotes>See https://github.com/elastic/elasticsearch-net/releases/tag/$version$</releaseNotes> | ||
| <copyright>2014-$year$ Elasticsearch BV</copyright> | ||
| <tags>elasticsearch,elastic,search,lucene,nest</tags> |
Contributor
There was a problem hiding this comment.
Suggested change
| <tags>elasticsearch,elastic,search,lucene,nest</tags> | |
| <tags>elasticsearch,elastic,search,lucene,nest,tests,testing</tags> |
| <iconUrl>https://raw.githubusercontent.com/elastic/elasticsearch-net/master/build/nuget-icon.png</iconUrl> | ||
| <requireLicenseAcceptance>false</requireLicenseAcceptance> | ||
| <summary>Provides a way to assert cluent behaviour through a rule engine backed VirtualClusterConnection</summary> | ||
| <description>Provides a way to assert cluent behaviour through a rule engine backed VirtualClusterConnection</description> |
Contributor
There was a problem hiding this comment.
Might be useful to also mention that it's useful for testing purposes?
Co-Authored-By: Russ Cam <russ.cam@elastic.co>
…lasticsearch-net into feature/7.x/virtual-connection
…r virtualized cluster project
Mpdreamz
added a commit
that referenced
this pull request
Sep 24, 2019
* rename Serializer subfolder to Auxiliary * introduced seperate project for virtual cluster * VirtualCluster auditor returns a more real fake response * moved WaitingInMemoryConnection back * Union was copied twice * make sure VirtualClusterConnection is exposed * Add Elasticsearch.Net.Virtual as project into the build * do not rely on HttpClientException on net461 * Exception test needs to be .NET core specific now * build for canary was still looking in Serializers folder not Auxiliary * canary handling for Elasticsearch.Net.Virtual * skip nuget testing for Elasticearch.Net.Virtual * reference TestPackageVersion of Elasticsearch.Net optionally * still included csproj * Use targetting pack and made sure canary works under linux too * Update build/Elasticsearch.Net.Virtual.nuspec Co-Authored-By: Russ Cam <russ.cam@elastic.co> * Client on VirtualizedCluster became public on 7.x, mirror that here * change namespace to Elasticsearch.Net.VirtualizedCluster * update tests from 7.x and bumped sdk version * bump global json back * canary build needs to resolve Elasticsearch.Net reference as nuget for virtualized cluster project * bulk all exception tests canary ignore * bulk all exception tests canary ignore * make test less strict * skip test on CI (cherry picked from commit 25c0019)
russcam
pushed a commit
that referenced
this pull request
Oct 1, 2019
* rename Serializer subfolder to Auxiliary * introduced seperate project for virtual cluster * VirtualCluster auditor returns a more real fake response * moved WaitingInMemoryConnection back * Union was copied twice * make sure VirtualClusterConnection is exposed * Add Elasticsearch.Net.Virtual as project into the build * do not rely on HttpClientException on net461 * Exception test needs to be .NET core specific now * build for canary was still looking in Serializers folder not Auxiliary * canary handling for Elasticsearch.Net.Virtual * skip nuget testing for Elasticearch.Net.Virtual * reference TestPackageVersion of Elasticsearch.Net optionally * still included csproj * Use targetting pack and made sure canary works under linux too * Update build/Elasticsearch.Net.Virtual.nuspec Co-Authored-By: Russ Cam <russ.cam@elastic.co> * Client on VirtualizedCluster became public on 7.x, mirror that here * change namespace to Elasticsearch.Net.VirtualizedCluster * update tests from 7.x and bumped sdk version * bump global json back * canary build needs to resolve Elasticsearch.Net reference as nuget for virtualized cluster project * bulk all exception tests canary ignore * bulk all exception tests canary ignore * make test less strict * skip test on CI
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moves our virtual clustering we use in tests into its own package.