[ https://issues.apache.org/jira/browse/CASSANDRA-15718?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17093529#comment-17093529 ]
Stephen Mallette commented on CASSANDRA-15718: ---------------------------------------------- [~djoshi] I'm fine with the introduction of {{Range}} there rather than using {{Pair}} +1 > 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 > Attachments: CASSANDRA-15582-test-cleanup.patch > > > 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