Skip to content

SAMZA-1196: Fix TestJmxReporter#140

Closed
shanthoosh wants to merge 1 commit intoapache:masterfrom
shanthoosh:fix-test-jmx-reporter
Closed

SAMZA-1196: Fix TestJmxReporter#140
shanthoosh wants to merge 1 commit intoapache:masterfrom
shanthoosh:fix-test-jmx-reporter

Conversation

@shanthoosh
Copy link
Contributor

No description provided.

@shanthoosh shanthoosh force-pushed the fix-test-jmx-reporter branch from 26a5bfd to 09239d1 Compare April 25, 2017 21:35
@shanthoosh
Copy link
Contributor Author

@prateekm @navina

Please take a look when you get chance.

Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be good to add another assert to the test to make sure metrics are registered from the listener. Otherwise LGTM, thanks.

@shanthoosh shanthoosh force-pushed the fix-test-jmx-reporter branch from 09239d1 to f27fda5 Compare April 25, 2017 22:44
Replace spawning and connecting to JMX Server with mocking.
@shanthoosh shanthoosh force-pushed the fix-test-jmx-reporter branch from f27fda5 to 25abff9 Compare April 25, 2017 22:57
@shanthoosh
Copy link
Contributor Author

@prateekm

Added additional asserts to test metrics reporting through listener.

Thanks.

Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

registry.newCounter(metricGroup, metricName)

assertTrue(stateViaJMX > 0)
assertEquals(1, registry.listeners.size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary, this is registry internal state.

@navina
Copy link
Contributor

navina commented Apr 28, 2017

lgtm! Thanks for fixing this 👍

@asfgit asfgit closed this in 9d66772 May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants