Refactor ExtensionsRunner test code out of production source tree#172
Refactor ExtensionsRunner test code out of production source tree#172dbwiddis merged 2 commits intoopensearch-project:mainfrom
Conversation
03c9cac to
d75df92
Compare
| super.setUp(); | ||
| this.extensionsRunner = new ExtensionsRunner(); | ||
| this.extensionsRunner = new ExtensionsRunnerForTest(); | ||
| this.initialTransportService = extensionsRunner.extensionTransportService; |
There was a problem hiding this comment.
The constructor of ExtensionRunner creates an instance of TransportService which listens a port in the background.
Tests further use setExtensionTransportService to replace the created instance with a mock to use in testing. The original instance becomes unreachable and our tearDown cannot stop it, which causes the rest of the tests to fail due to the port being already in use. Hence, this hack.
IMO, we should pass TransportService as a constructor parameter and not instantiate inside.
There was a problem hiding this comment.
At this point, let's "hack" as needed to get this merged.
I agree with you and have questioned how we've tracked the transport service from some of the initial PRs. I'm not sure putting it in the constructor is the best solution but we do need something better than what we have.
We do have a future requirement to gracefully handle disconnections/reconnections (hot plugin, etc.) and I think handling these will definitely need to be part of that functionality as well.
dbwiddis
left a comment
There was a problem hiding this comment.
Code changes look good! Thank you for your contribution and your patience in dealing with our early-stage code!
Please undo the whitespace changes in build.gradle and add the DCO signoff and I'll approve this.
| * @throws IOException if the runner failed to read settings or API. | ||
| */ | ||
| private ExtensionsRunner(Extension extension) throws IOException { | ||
| protected ExtensionsRunner(Extension extension) throws IOException { |
There was a problem hiding this comment.
Note to self/other reviewers: package private would be sufficient for tests if we only want to make ExtensionsRunner extendable for testing. However I think it's reasonable to think any user might wish to create a simple anonymous extension (possibly for their own testing!) so I like using protected here.
Signed-off-by: Priyambada Roul <roulpriyambada@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
| @Override | ||
| @AfterEach | ||
| public void tearDown() throws Exception { | ||
| super.tearDown(); |
There was a problem hiding this comment.
Thanks for adding the tearDown. This will definitely avoid the bind connection error.
| @@ -0,0 +1,32 @@ | |||
| package org.opensearch.sdk; | |||
There was a problem hiding this comment.
This is great!
It sets up our initial framework for tests going forward.
saratvemulapalli
left a comment
There was a problem hiding this comment.
Thanks @roulpriya for the changes!
…t#172) Signed-off-by: Priyambada Roul <roulpriyambada@gmail.com>
…t#172) Signed-off-by: Priyambada Roul <roulpriyambada@gmail.com>
Description
Remove
ExtensionsRunnerconstructor which was used only in testsIssues Resolved
Closes #171
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.