-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve Tests #114
Improve Tests #114
Conversation
…s into improve-test-coverage
Code looks good. Will verify the code now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified. Everything seems to be working properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public void testInventoryAccessCountMetric() {
metrics = getMetrics();
int accessCountBefore = getIntMetric(metrics,
"application_inventoryAccessCount_total");
int accessCountAfter = 0;
connectToEndpoint(baseHttpUrl + INVENTORY_HOSTS);
metrics = getMetrics();
accessCountAfter = getIntMetric(metrics,
"application_inventoryAccessCount_total");
assertTrue(accessCountAfter > accessCountBefore);
}
to
public void testInventoryAccessCountMetric() {
metrics = getMetrics();
int accessCountBefore = getIntMetric(metrics,
"application_inventoryAccessCount_total");
connectToEndpoint(baseHttpUrl + INVENTORY_HOSTS);
metrics = getMetrics();
int accessCountAfter = getIntMetric(metrics,
"application_inventoryAccessCount_total");
assertTrue(accessCountAfter > accessCountBefore);
}
@AustinSeto The logic was changed for your new implementation. The existing way checks all metrics startedWith a string, but your |
@gkwan-ibm updated test implementation to use map to store all found metrics and compare all of them. |
hi @Charlotte-Holt would you review the README.adoc? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AustinSeto Just a few ID review comments. Let me know if you have questions!
README.adoc
Outdated
`\http://localhost:9080/inventory/systems` URL to retrieve the whole inventory, and then it asserts | ||
that this endpoint is accessed at least once. | ||
* The [hotspot=testInventoryAccessCountMetric file=0]`testInventoryAccessCountMetric()` test case validates the [hotspot=countedForList file=1]`@Counted` metric. | ||
The test case obtains the metric before and after a request to the `\http://localhost:9080/inventory/systems` URL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggested update
The test case obtains metric data before and after a request to the
\http://localhost:9080/inventory/systems
URL.
README.adoc
Outdated
that this endpoint is accessed at least once. | ||
* The [hotspot=testInventoryAccessCountMetric file=0]`testInventoryAccessCountMetric()` test case validates the [hotspot=countedForList file=1]`@Counted` metric. | ||
The test case obtains the metric before and after a request to the `\http://localhost:9080/inventory/systems` URL. | ||
It asserts that the metric was increased after the access. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this one, I'm thinking update it to
It then asserts that the metric was increased after the URL was accessed.
If that makes sense to you? I had a harder time understanding exactly what this sentence meant.
@Charlotte-Holt I made both suggested changes, let me know if there's any other comments. |
Addresses #64
Restores changes from #91