[
https://issues.apache.org/jira/browse/CASSANDRA-15718?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17082376#comment-17082376
]
Stephen Mallette commented on CASSANDRA-15718:
----------------------------------------------
[~dcapwell] I've started making adjustments to use quick theories and
interestingly the assertion of the histogram fails as you point out that the
"current test is wrong". I started thinking about those assertions:
{code}
assertTrue(assertionMessage,partitionsPerLoggedBatchCountPre <=
metrics.partitionsPerLoggedBatch.getSnapshot().getMax());
assertTrue(assertionMessage,partitionsPerUnloggedBatchCountPre <=
metrics.partitionsPerUnloggedBatch.getSnapshot().getMax());
assertTrue(assertionMessage,partitionsPerCounterBatchCountPre <=
metrics.partitionsPerCounterBatch.getSnapshot().getMax());
{code}
and started to wonder if the histograms need to be asserted at all. I mostly
just copied the semantics of the old tests, but didn't really stop to consider
them. Is the assertion of the histogram fulfilling any purpose in your view
compared to what is already being asserted in terms of the direct expectations
of the metrics:
{code}
assertEquals(assertionMessage,partitionsPerUnloggedBatchCountPost,
metrics.partitionsPerUnloggedBatch.getCount());
assertEquals(assertionMessage, partitionsPerLoggedBatchCountPost,
metrics.partitionsPerLoggedBatch.getCount());
assertEquals(assertionMessage, partitionsPerCounterBatchCountPost,
metrics.partitionsPerCounterBatch.getCount());
{code}
> Improve BatchMetricsTest
> -------------------------
>
> Key: CASSANDRA-15718
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15718
> Project: Cassandra
> Issue Type: Improvement
> Components: Test/unit
> Reporter: Stephen Mallette
> Assignee: Stephen Mallette
> Priority: Normal
>
> As noted in CASSANDRA-15582 {{BatchMetricsTest}} should test
> {{BatchStatement.Type.COUNTER}} to cover all the {{BatchMetrics}}. Some
> changes were introduced to make this improvement at:
> https://github.com/apache/cassandra/compare/trunk...spmallette:CASSANDRA-15582-trunk-batchmetrics
> and the following suggestions were made in review (in addition to the
> suggestion that a separate JIRA be created for this change) by [~dcapwell]:
> {quote}
> * I like the usage of BatchStatement.Type for the tests
> * honestly feel quick theories is better than random, but glad you added the
> seed to all asserts =). Would still be better as a quick theories test since
> you basically wrote a property anyways!
> *
> https://github.com/apache/cassandra/compare/trunk...spmallette:CASSANDRA-15582-trunk-batchmetrics#diff-8948cec1f9d33f10b15c38de80141548R131
> feel you should rename to expectedPartitionsPerLoggedBatch
> {Count,Logged,Unlogged}
> * . pre is what the value is, post is what the value is expected to be
> (rather than what it is).
> *
> *
> https://github.com/apache/cassandra/compare/trunk...spmallette:CASSANDRA-15582-trunk-batchmetrics#diff-8948cec1f9d33f10b15c38de80141548R150
> this doesn't look correct. the batch has distinctPartitions mutations, so
> shouldn't max reflect that? I ran the current test in a debugger and see that
> that is the case (aka current test is wrong).
> most of the comments are nit picks, but the last one looks like a test bug to
> me
> {quote}
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
