[jira] [Commented] (CASSANDRA-15718) Improve BatchMetricsTest

["Stephen Mallette (Jira)" <ji...@apache.org>]

    [ 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-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org