henrikingo commented on code in PR #2284:
URL: https://github.com/apache/cassandra/pull/2284#discussion_r1174247473
##########
test/unit/org/apache/cassandra/db/DirectoriesTest.java:
##########
@@ -696,4 +732,144 @@ private List<Directories.DataDirectoryCandidate>
getWriteableDirectories(DataDir
return candidates;
}
+ private int getDiyThreadId()
+ {
+ return myDiyId = diyThreadId.getAndIncrement();
+ }
+
+ private void detachLogger()
+ {
+ logger.detachAppender(listAppender);
+ MDC.remove(this.MDCID);
+ }
+
+ private void tailLogs()
+ {
+ int diyId = getDiyThreadId();
+ MDC.put(this.MDCID, String.valueOf(diyId));
+ logger = (Logger) LoggerFactory.getLogger(Directories.class);
+
+ // create and start a ListAppender
+ listAppender = new ListAppender<>();
+ listAppender.start();
+
+ // add the appender to the logger
+ logger.addAppender(listAppender);
+ }
+
+ private List<ILoggingEvent> filterLogByDiyId(List<ILoggingEvent> log)
+ {
+ ArrayList<ILoggingEvent> filteredLog = new ArrayList<>();
+ for(ILoggingEvent event : log)
+ {
+ int mdcId =
Integer.parseInt(event.getMDCPropertyMap().get(this.MDCID));
+ if(mdcId == myDiyId){
+ filteredLog.add(event);
+ }
+ }
+ return filteredLog;
+ }
+
+ private void checkFormattedMessage(List<ILoggingEvent> log, Level
expectedLevel, String expectedMessage, int expectedCount)
+ {
+ int found=0;
+ for(ILoggingEvent e: log)
+ {
+ System.err.println(e.getFormattedMessage());
+ if(e.getFormattedMessage().endsWith(expectedMessage))
+ {
+ if (e.getLevel() == expectedLevel)
+ found++;
+ }
+ }
+
+ assertEquals(expectedCount, found);
+ }
+
+ @Test
+ public void testHasAvailableDiskSpace()
+ {
+ DataDirectory[] dataDirectories = new DataDirectory[]
Review Comment:
Hmm... I don't think that's equivalent to the test as submitted in the PR.
First of all the dataDirectories should all exist together in each test. They
aren't different input values for a parameterized test.
From there, note that the lines
String logMsgFormat = "DataDirectory %s has %d bytes available,
checking if we can write %d bytes";
String logMsg = String.format(logMsgFormat, "/nearlyFullDir1", 11,
2);
checkFormattedMessage(filteredLog, Level.DEBUG, logMsg, 1);
Are merely checking log output from a test that already ran. If we wanted to
parameterize, we can, but that would be just these 4 lines
assertTrue(d.hasAvailableDiskSpace(1,2));
assertTrue(d.hasAvailableDiskSpace(10,99));
assertFalse(d.hasAvailableDiskSpace(10,1024));
assertFalse(d.hasAvailableDiskSpace(1024,1024*1024));
And in the log output checking code, only 1 value could be parameterized.
(The number 2 in what I quoted above.)
In short,I don't see the benefit of Parameterized here. The question
remains: If I wrap the assertions into a simple loop:
Collection<Object[]> checkTheseOut = Arrays.asList(new Object[]{
Level.DEBUG, "/nearlyFullDir1", 11, 2},
new Object[]{ "/uniformDir2, 999, 2 },
...);
for ( values: checkTheseOut )
{
String logMsg = String.format(logMsgFormat, (String)values[1],
(int)values[2], (int)values[3]);
checkFormattedMessage(filteredLog, (Level)values[0], logMsg,
1);
}
...then how will the future developer running this unit test easily
interpret the results when something failed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
