Skip to content

Conversation

@texastony
Copy link
Contributor

@texastony texastony commented May 28, 2025

Issue #, if available:

Description of changes:

Integ Tests use CFN Roles created by this PR to ensure that the Branch Key Store correctly handles EC.

Squash/merge commit message, if applicable:

test(Dafny): BKS Integ Test ensure EncCtx is prefixed/unprefixed as expected

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@texastony texastony marked this pull request as ready for review May 28, 2025 19:25
@texastony texastony requested a review from a team as a code owner May 28, 2025 19:25
@texastony texastony force-pushed the texastony/hv-2/test-JSON-Encoding-of-Mutations-Supports-Escape-Characters branch from a3e5c0a to a5d979d Compare May 28, 2025 20:50
Base automatically changed from texastony/hv-2/test-JSON-Encoding-of-Mutations-Supports-Escape-Characters to hv-2/hv-2 May 28, 2025 21:40
@texastony texastony force-pushed the texastony/hv-2/test-bks-ec-prefixed-defixed-as-expected branch from dcdf298 to 0cbccb5 Compare May 28, 2025 21:42
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non blocking comment: createHV1, checkBranchKey and mutateToHV2 is very identical to the functions in TestMutationsWithEscapeCharactersInEC.java. The only thing that different is the EC. We could had made one helper function and call helper function here and in TestMutationsWithEscapeCharactersInEC.java.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
I thought about that.
Or made collapse all three of these class into a super and it's extensions.

But ultimately, the methods here are calling already created helper methods.
I am for abstraction, but not mega abstraction.

I think this walks a good balance.

Copy link
Member

@rishav-karanjit rishav-karanjit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@texastony texastony merged commit bd3124d into hv-2/hv-2 May 28, 2025
114 checks passed
@texastony texastony deleted the texastony/hv-2/test-bks-ec-prefixed-defixed-as-expected branch May 28, 2025 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants