KAFKA-16888 Fix failed StorageToolTest.testFormatSucceedsIfAllDirectoriesAreAvailable and StorageToolTest.testFormatEmptyDirectory#16186
Conversation
…riesAreAvailable and StorageToolTest.testFormatEmptyDirectory
There was a problem hiding this comment.
Thanks for the fix @brandboat ! I assume the patch fixes the broken tests, but could you please add some info in the PR description for reviewers know why this fixes the issue. Something like:
- Why it is broken
- In which commit changes the behavior and why changes
- How do you fix it
Thanks.
chia7712
left a comment
chia7712
left a comment
There was a problem hiding this comment.
@brandboat thanks for this hot-fix!
| val actual = stream.toString().split("\\r?\\n") | ||
| val actual = stream.toString().split("\\r?\\n").drop(1) | ||
| val expect = availableDirs.map("Formatting %s".format(_)) | ||
| assertEquals(availableDirs.size, actual.size) |
There was a problem hiding this comment.
It seems to me this (failed) check can be removed. All we wan to test is the folders get formatted only once, and that is verified in the following assert.
There was a problem hiding this comment.
I've addressed comments in the latest commit, thanks for reviewing.
| assertEquals(0, StorageTool. | ||
| formatCommand(new PrintStream(stream), Seq(tempDir.toString), metaProperties, bootstrapMetadata, MetadataVersion.latestTesting(), ignoreFormatted = false)) | ||
| assertTrue(stream.toString().startsWith("Formatting %s".format(tempDir))) | ||
| assertTrue(stream.toString().split("\\r?\\n").drop(1)(0).startsWith("Formatting %s".format(tempDir))) |
There was a problem hiding this comment.
Maybe we can check that output contains the expected content. for example:
assertTrue(stream.toString().split("\\r?\\n").exists(_.startsWith("Formatting %s".format(tempDir))))drop(1)(0) is too magic to understand to me :)
| assertEquals(0, StorageTool. | ||
| formatCommand(new PrintStream(stream2), Seq(tempDir.toString), metaProperties, bootstrapMetadata, MetadataVersion.latestTesting(), ignoreFormatted = true)) | ||
| assertEquals("All of the log directories are already formatted.%n".format(), stream2.toString()) | ||
| assertEquals("All of the log directories are already formatted.".format(), stream2.toString().split("\\r?\\n").drop(1)(0)) |
Added some description in pr, thanks ! 😃 |
…riesAreAvailable and StorageToolTest.testFormatEmptyDirectory (#16186) Reviewers: Luke Chen <showuon@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
…riesAreAvailable and StorageToolTest.testFormatEmptyDirectory (apache#16186) Reviewers: Luke Chen <showuon@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
…riesAreAvailable and StorageToolTest.testFormatEmptyDirectory (apache#16186) Reviewers: Luke Chen <showuon@gmail.com>, Chia-Ping Tsai <chia7712@gmail.com>
3 participants
a quick fix for https://issues.apache.org/jira/browse/KAFKA-16888
459da47#diff-4bacfdbf0e63a4d5f3deb1a0d39037a18510ac24ee5ec276fe70bc818ba4d209L505 put extra information as follows to the beginning of the output which break some tests in StorageToolTest that rely on the original output.
This pr modified the test
testFormatSucceedsIfAllDirectoriesAreAvailable,testFormatEmptyDirectoryto get rid of those extra infos to avoid test failed.Committer Checklist (excluded from commit message)