Adding Test coverage to NoOpPinotCrypt class#5726
Adding Test coverage to NoOpPinotCrypt class#5726rush00121 wants to merge 5 commits intoapache:masterfrom
Conversation
pinot-spi/src/test/java/org/apache/pinot/spi/crypt/NoOpPinotCryptTest.java
Show resolved
Hide resolved
| Assert.assertTrue(FileUtils.contentEquals(encryptedFile, decryptedFile)); | ||
| decryptedFile = new File("fake"); | ||
| pinotCrypter.encrypt(decryptedFile, encryptedFile); | ||
| Assert.assertFalse(encryptedFile.exists()); |
There was a problem hiding this comment.
Seems like this test only asserts that an output file is generated. IMHO, a better test would be one that ensures that we get the same original content back after encryption -> decryption. This might mean that we just have one test method as opposed to two tests.
There was a problem hiding this comment.
Sure, makes sense. I have combined the 2 to a single test. Where we are comparing the srcFile to the final decryptedFile.
Also have separated the invalid scenarios to their respective tests
|
|
||
| public class NoOpPinotCryptTest { | ||
|
|
||
| PinotCrypter pinotCrypter; |
There was a problem hiding this comment.
As per Pinot Coding Style, member variables should be prefixed with _.
| public void testInvalidEncryption() throws IOException { | ||
| srcFile = new File("fake"); | ||
| pinotCrypter.encrypt(srcFile, decryptedFile); | ||
| Assert.assertFalse(decryptedFile.exists()); |
There was a problem hiding this comment.
Isn't decryptedFile already created in init()? Which also brings up the point that the files created in init() would only end up being used in one of the tests, so what's the purpose of creating them in init()?
|
not valid |
5 participants
Description
This class did not have any code coverage. Adding a simple test to improve test coverage.
Adding test as part of this issue: #5699
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
Does this PR fix a zero-downtime upgrade introduced earlier?
Does this PR otherwise need attention when creating release notes? No