[Tests] Mutualize fixtures code in BaseHttpFixture #31210
Conversation
Many fixtures have similar code for writing the pid & ports files or for handling HTTP requests. This commit changes the current ExampleFixture in order to make it an abstract BaseHttpFixture class that can be extended for specific testing purposes. This class provides some simple Request and Response classes to represent the HTTP requests and responses and takes care of writing the pid and ports files and starting the HTTP server. It allows to remove lot of duplicated code and forbidden APIs invocations. The current AmazonS3Fixture, GoogleCloudStorageFixture, AzureStorageFixture, and URLFixture have been removed. The AmazonS3TestServer, , AzureStorageTestServer, GoogleCloudStorageTestServer have been renamed and now extend BaseHttpFixture. This commit also fixes an issue with the AntFixture that was expecting a false result for the waitCondition(), whereas it should expect `true`.
tlrx
added
the
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed |
| boolean success | ||
| try { | ||
| success = waitCondition(this, ant) == false | ||
| success = waitCondition(this, ant) |
There was a problem hiding this comment.
lol, good catch. Can you create a separate PR for this?
modules/reindex/build.gradle
Outdated
| @@ -121,6 +121,10 @@ if (Os.isFamily(Os.FAMILY_WINDOWS)) { | |||
| baseDir, | |||
| unzip.temporaryDir, | |||
| version == '090' | |||
| waitCondition = { fixture, ant -> | |||
| return fixture.portsFile.exists() | |||
There was a problem hiding this comment.
The OldElasticsearch fixture used in the reindex module tests starts an Elasticsearch instance and waits for its HTTP service to be started, then it writes the HTTP port to the ports file.
Before this change, the fixture here used the default AntFixture's waitCondition which is to execute a HTTP GET request on the address indicated in the fixture's ports file. Most fixtures write a IP address and a port number (ex: 127.0.0.1:48356) in this file but OldElasticsearch only writes the Elasticsearch HTTP port. Without this change, the fixture executes a GET http://9200 request and fails directly, which was unnoticed because the bug in the waitCondition(this, ant) == false evaluation.
I'll revert this change in this PR and address all changes related to waitCondition(this, ant) == false in another PR as your suggested.
| /** | ||
| * Base class for test fixtures that requires a {@link HttpServer} to work. | ||
| */ | ||
| public abstract class BaseHttpFixture { |
There was a problem hiding this comment.
should we put this into test:framework instead?
There was a problem hiding this comment.
But that would mean to bring back all the test framework classes and dependencies when we just want to start a single class to simulate an external service. Thinking about it twice, I don't think that's a good idea.
There was a problem hiding this comment.
sure, but looking at your changes here: https://github.com/elastic/elasticsearch/pull/31210/files#diff-59ba8e13c56cbfc8a1bad12a2c8df5b0R32
it seems that you've done exactly that, i.e., mix the fixture classpath into the test classpath.
Also, if we now have a separate project (base-fixture), we'll need to publish that dependency as well so that others can use it.
There was a problem hiding this comment.
it seems that you've done exactly that, i.e., mix the fixture classpath into the test classpath.
Yes, that is more a consequence of the fixture originally using a test server that was shared with other test class, and now with dedicated projects I could have move the fixture code to the main sources insead of test sources.
Anyway, moving the BasicFixture to the test framework simplifies things and, as you said, it also allows others to use it. So I did that.
|
@ywelsch Thanks for the review. I moved BasicFixture to the test framework and I renamed it AbstractHttpFixture. It simplifies things. I also merged master to get a chance to have CI passed. Can you have another look please? |
|
Thanks @ywelsch |
* elastic/master: More detailed tracing when writing metadata (elastic#31319) [Tests] Mutualize fixtures code in BaseHttpFixture (elastic#31210) Remove RestGetAllAliasesAction (elastic#31308) Temporary fix for broken build Reenable Checkstyle's unused import rule (elastic#31270) Remove remaining unused imports before merging elastic#31270 Fix non-REST doc snippet
Many fixtures have similar code for writing the pid & ports files or for handling HTTP requests. This commit adds an AbstractHttpFixture class in the test framework that can be extended for specific testing purposes.
* 6.x: SQL: Fix build on Java 10 [Tests] Mutualize fixtures code in BaseHttpFixture (#31210) [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect [ML] Update test thresholds to account for changes to memory control (#31289) Reenable Checkstyle's unused import rule (#31270) [ML] Check licence when datafeeds use cross cluster search (#31247) Fix non-REST doc snippet [DOC] Extend SQL docs [DOCS] Shortens ML API intros Use quotes in the call invocation (#31249) move security ingest processors to a sub ingest directory (#31306) SQL: Whitelist SQL utility class for better scripting (#30681) Add 5.6.11 version constant. Fix version detection. [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299) Add missing release notes. Security: fix token bwc with pre 6.0.0-beta2 (#31254) Fix compilation error in UpdateSettingsIT (#31304) Test: Remove broken yml test feature (#31255) Add unreleased version 6.3.1 [Rollup] Metric config parser must use builder so validation runs (#31159) Removes experimental tag from scripted_metric aggregation (#31298) [DOCS] Removes coming tag from 6.3.0 release notes 6.3 release notes. Add notion of internal index settings (#31286) REST high-level client: add Cluster Health API (#29331) Remove leftover usage of deprecated client API SyncedFlushResponse to implement ToXContentObject (#31155) Add Get Aliases API to the high-level REST client (#28799) HLRest: Add get index templates API (#31161) Log warnings when cluster state publication failed to some nodes (#31233) Fix AntFixture waiting condition (#31272) [TEST] Mute RecoveryIT.testHistoryUUIDIsGenerated Ignore numeric shard count if waiting for ALL (#31265) Update checkstyle to 8.10.1 (#31269) Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202) Revert upgrade to Netty 4.1.25.Final (#31282) Use armored input stream for reading public key (#31229) [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes #28008 (#31160) Fix Netty 4 Server Transport tests. Again. [DOCS] Fixed typo. [DOCS] Added release highlights for 6.3 (#31256) [DOCS] Mark SQL feature as experimental [DOCS] Updates machine learning custom URL screenshots (#31222) Fix naming conventions check for XPackTestCase Fix security Netty 4 transport tests Fix race in clear scroll (#31259) [DOCS] Clarify audit index settings when remote indexing (#30923) [ML][TEST] Mute tests using rules (#31204) Support RequestedAuthnContext (#31238) Validate xContentType in PutWatchRequest. (#31088) [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024) Upgrade to Netty 4.1.25.Final (#31232) Suppress extras FS on caching directory tests Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)" Revert "Fix snippets in upgrade docs" Fix snippets in upgrade docs [DOCS] Added 6.3 info & updated the upgrade table. (#30940) Enable custom credentials for core REST tests (#31235) Move ESIndexLevelReplicationTestCase to test framework (#31243) Encapsulate Translog in Engine (#31220) [DOCS] Adds machine learning 6.3.0 release notes (#31217) Remove all unused imports and fix CRLF (#31207) [TEST] Fix testRecoveryAfterPrimaryPromotion [Docs] Remove mention pattern files in Grok processor (#31170) Use stronger write-once semantics for Azure repository (#30437) Don't swallow exceptions on replication (#31179) Compliant SAML Response destination check (#31175) Move java version checker back to its own jar (#30708) TEST: Retry synced-flush if ongoing ops on primary (#30978) [test] add fix for rare virtualbox error (#31212)
* master: 992c788 Uncouple persistent task state and status (#31031) 8c6ee7d Describe how to add a plugin in Dockerfile (#31340) 1c5cec0 Remove http status code maps (#31350) 87a676e Do not set vm.max_map_count when unnecessary (#31285) e5b7137 TEST: getCapturedRequestsAndClear should be atomic (#31312) 0324103 Painless: Fix bug for static method calls on interfaces (#31348) d6d0727 QA: Fix resolution of default distribution (#31351) fcf1e41 Extract common http logic to server (#31311) 6dd81ea Build: Fix the license in the pom zip and tar (#31336) 8f886cd Treat ack timeout more like a publish timeout (#31303) 9b29327 [ML] Add description to ML filters (#31330) f7a0caf SQL: Fix build on Java 10 375d09c [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect 4877cec More detailed tracing when writing metadata (#31319) bbfe1ec [Tests] Mutualize fixtures code in BaseHttpFixture (#31210)
Many fixtures have similar code for writing the pid & ports files or for handling HTTP requests. This commit changes the current ExampleFixture in order to make it an abstract BaseHttpFixture class that can be extended for specific testing purposes. This class provides some simple Request and Response classes to represent the HTTP requests and responses and takes care of writing the pid and ports files and starting the HTTP server. It allows to remove duplicated code and forbidden APIs invocations.
The current AmazonS3Fixture, GoogleCloudStorageFixture, AzureStorageFixture, and URLFixture have been removed. The AmazonS3TestServer, AzureStorageTestServer, GoogleCloudStorageTestServer have been renamed and now extend BaseHttpFixture.
This commit also fixes an issue with the AntFixture that was expecting a false result for the waitCondition(), whereas it should expect
true.