Skip to content

test: Condition tests for caching-materials-manager-browser #177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

seebees
Copy link
Contributor

@seebees seebees commented Jul 30, 2019

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

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

@seebees seebees requested a review from a team July 30, 2019 04:34
@@ -59,4 +59,28 @@ describe('WebCryptoCachingMaterialsManager', () => {
expect(test._maxBytesEncrypted).to.equal(maxBytesEncrypted)
expect(test._maxMessagesEncrypted).to.equal(maxMessagesEncrypted)
})

it('Precondition: A partition value must exist for WebCryptoCachingMaterialsManager.', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this test also need to be added to caching_materials_manager_node.test.ts ?

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, and it is in a different PR

/* Binary data is being transformed to utf-8.
* This means it is highly likely to be less than 64 characters long.
*/
expect(test._partition).to.be.a('string').and.to.have.length.greaterThan(50)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the likelihood that 64 bytes of random data -> UTF-8 will be less than 50 characters? Enough that we will "never" see this test fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sigh... I "fixed" this in https://github.com/awslabs/aws-encryption-sdk-javascript/pull/178/files#diff-2e3dd129c30c2285d72b61362efd41b9R58

Once that is merged. I'll update the test... utf8 on random data is.... sub optimal.

maxAge
})
/* Binary data is being transformed to utf-8.
* This means it is highly likely to be less than 64 characters long.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is no longer needed

@seebees seebees merged commit f886c83 into aws:master Aug 2, 2019
@seebees seebees deleted the conditions-caching-materials-manager-browser branch August 2, 2019 23:31
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