chore: add instruction file manipulation test#119
Merged
josecorella merged 5 commits intoDec 15, 2025
Merged
Conversation
seebees
requested changes
Dec 12, 2025
seebees
left a comment
seebees
left a comment
There was a problem hiding this comment.
I think that the format should get changed.
It would be nice to be a little clearer about what exactly is being manipulated.
Comment on lines
+824
to
+829
| // At high concurrency, this test tends to get: | ||
| // BadDigest Message: The CRC64NVME you specified did not match the calculated checksum. | ||
| // I think this is a read after write issue. | ||
| // A better fix, would be to break this tests suite up into encrypt/decrypt | ||
| // rather than having a test for many pairs and doing encrypt/decrypt on each pair | ||
| Thread.sleep(100); |
There was a problem hiding this comment.
We don't need these. They should get taken out.
|
|
||
| @ParameterizedTest(name = "{displayName} for Encrypt: {0}, Decrypt: {1}") | ||
| @MethodSource("software.amazon.encryption.s3.TestUtils#encryptImprovedDecryptImproved") | ||
| public void GIVEN_KCGCMEncryptedData_AND_ImprovedClientDecryptingWithForbidEncryptAllowDecrypt_WHEN_Decrypt_THEN_Pass( |
There was a problem hiding this comment.
This test tends to have an encryp, then decrypt step. So you don't need to do every pair.
josecorella
force-pushed
the
jocorell/manipulate-instruction-file-tests
branch
from
31b476e to
25ec63d
Compare
seebees
previously requested changes
Dec 12, 2025
seebees
left a comment
seebees
left a comment
There was a problem hiding this comment.
I would remove the outs. but otherwise looks good.
| @ParameterizedTest(name = "{0}: Fail to decrypt with manipulated V3 Instruction File") | ||
| @MethodSource("software.amazon.encryption.s3.InstructionFileFailures$DecryptTests#clientsCanGetKMSWithInstructionFile") | ||
| void decryptWithManipulatedInstructionFileV3ImprovedClients(TestUtils.LanguageServerTarget language) { | ||
| if (crossLanguageObjectsInstructionFileManipulatedV3.isEmpty()) return; |
There was a problem hiding this comment.
I would remove this. I think we would rather fail than silently not test this.
Suggested change
| if (crossLanguageObjectsInstructionFileManipulatedV3.isEmpty()) return; |
| @ParameterizedTest(name = "{0}: Fail to decrypt with manipulated V2 Instruction File") | ||
| @MethodSource("software.amazon.encryption.s3.InstructionFileFailures$DecryptTests#clientsCanGetKMSWithInstructionFile") | ||
| void decryptWithManipulatedInstructionFileV2ImprovedClients(TestUtils.LanguageServerTarget language) { | ||
| if (crossLanguageObjectsInstructionFileManipulatedV2.isEmpty()) return; |
There was a problem hiding this comment.
Same here
Suggested change
| if (crossLanguageObjectsInstructionFileManipulatedV2.isEmpty()) return; |
kessplas
approved these changes
Dec 15, 2025
kessplas
dismissed
seebees’s stale review
seebees is OOO, i've verified the requested changes were made
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.