Refactor ExtensionsRunner test code out of production source tree #172
Conversation
dbwiddis
added
the
hacktoberfest
roulpriya
force-pushed
the
Issue-171
branch
2 times, most recently
from
03c9cac to
d75df92
Compare
| @Override | ||
| @BeforeEach | ||
| public void setUp() throws Exception { | ||
| 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.
roulpriya
changed the title
| /** | ||
| * Instantiates a new Extensions Runner using the specified extension. | ||
| * | ||
| * @param extension The settings with which to start the runner. | ||
| * @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 <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
| @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.
…t#172) Signed-off-by: Priyambada Roul <[email protected]>
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.