Fix failures in ResourceHeatMapGraphTest #321
Conversation
When ResourceHeatMapGraphTest was run after PerformanceAnalyzerWebServerTest, we noticed connection refused exceptions that didn't happen when the tests were run in isolation. This commit makes sure that PerformanceAnalyzerWebServerTest cleans up after itself so that subsequent tests will run as expected.
Codecov Report
@@ Coverage Diff @@
## master #321 +/- ##
============================================
+ Coverage 67.18% 67.29% +0.10%
- Complexity 1971 2004 +33
============================================
Files 288 291 +3
Lines 12804 12924 +120
Branches 1044 1057 +13
============================================
+ Hits 8603 8697 +94
- Misses 3838 3853 +15
- Partials 363 374 +11
Continue to review full report at Codecov.
|
| @@ -70,9 +70,13 @@ public void tearDown() { | |||
| // Unset all SSL settings | |||
| if (oldBindHost != null) { | |||
| PluginSettings.instance().overrideProperty(PerformanceAnalyzerWebServer.WEBSERVICE_BIND_HOST_NAME, oldBindHost); | |||
| } else { | |||
| PluginSettings.instance().overrideProperty(PerformanceAnalyzerWebServer.WEBSERVICE_BIND_HOST_NAME, "localhost"); | |||
There was a problem hiding this comment.
Are we trying to restore oldBindHost to some default value here? If so, should it be done in setUp() ?
Same for others.
There was a problem hiding this comment.
Ah wait, this is for across tests. Got it.
There was a problem hiding this comment.
Let's take oldBindHost as an example. If oldBindHost is null then after this test class ran, we would previously keep the value that PerformanceAnalyzerWebServer changed WEBSERVICE_BIND_HOST_NAME to. This could lead to other tests failing.
The reason this runs as part of a tearDown is to make sure that everything is reset after the test class finishes running.
There was a problem hiding this comment.
Thanks for fixing this @sidheart . Minor qn for my understanding.
| @@ -151,6 +150,12 @@ public static void shutdown() { | |||
| clientServers.getHttpServer().stop(0); | |||
| clientServers.getNetServer().stop(); | |||
| clientServers.getNetClient().stop(); | |||
|
|
|||
| try { | |||
| Thread.sleep(1000); | |||
There was a problem hiding this comment.
Why is this sleep needed?
There was a problem hiding this comment.
It isn't strictly necessary, I'm mimicking similar logic from RcaControllerTest which does this sleep after stopping its clients and servers.
There was a problem hiding this comment.
nit: Maybe it’s a good idea to abstract the shutdown logic with sleep into its own method so that we don’t have to carry the sleep change wherever we use shutdown?
Calls to Assert.fail() obscure the actual Exception in our testing logs. This commit removes the fail() calls and simply throws instead.
Issue #: #322
Description of changes:
When ResourceHeatMapGraphTest was run after
PerformanceAnalyzerWebServerTest, we noticed connection refused
exceptions that didn't happen when the tests were run in isolation.
This commit makes sure that PerformanceAnalyzerWebServerTest cleans up
after itself so that subsequent tests will run as expected.
Tests:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.